Bug in QualTypeNames.cpp and adding an option to prepend "::" to fully qualified names.

Sat May 23 13:22:28 PDT 2020

Hi,

should we submit this CL, adding the option to prepend classes and struct
fully qualified names with "::"?

This can then allow people to slowly switch to the new function, and I can
close my fix/close my bug in CLIF. I can do more on this CL if it is
needed, but I would like to be sure this will get submitted and I will not
waste my time, and know exactly what you want me to do.

Possible options include:
- adding the full context, with the bug, in the CL description
- adding documentation on `getFullyQualifiedName` expliciting in which
context the bug can occur
- adding a FullyQualifiedTypePrinter() that will take an AST or a
TypePolicy, and set the correct fields, so one can do
QualType.getAsString(FullyQualifiedTypePrinter(ast)) to replace the
hereabove one.
- add some tests, but I do not know where and how.

When this few lines are submitted, I can update google3 for clang, and I
can start changing some internal occurrences, and finally fix the CLIF bug.

Thanks!

On Tue, May 12, 2020 at 11:19 PM Jean-Baptiste Lespiau <jblespiau at google.com>
wrote:

>
>
> On Tue, May 12, 2020 at 11:11 PM David Blaikie <dblaikie at gmail.com> wrote:
>
>> On Tue, May 12, 2020 at 12:40 PM Jean-Baptiste Lespiau <
>> jblespiau at google.com> wrote:
>>
>>> Hi,
>>>
>>> thanks for your answer.
>>>
>>> Just a few remarks:
>>>
>>> 1. I imagine that we cannot know if people are using
>>> getFullyQualifiedName. In particular, people could have developed their own
>>> internal tools, thus we cannot be aware of all callers. I do not know
>>> Clang's policy, but can we delete or completely change a function without
>>> deprecating it first?
>>>
>>
>> The LLVM project offers little/no API guarantees - potentially/especially
>> for a function with no use in-tree. But, yes, as Googlers we'd be
>> encouraged not to commit an API change knowing this might cause widespread
>> internal breakage without a plan/pre-emptively fixing things.
>>
>>
>>> I was imagining that the process was to deprecate it, document the case
>>> where it can be incorrect, and that in a next release it gets
>>> deleted/replaced (or someone steps up to fix it).
>>>
>>
>> Sometimes deprecation is used - certain APIs have a lot of out of tree
>> surface area
>>
>>
>>> 2. As example of different behaviors:
>>> (a) the qual_type.getAsString() will print implementation namespace
>>> details (e.g. ::std*::__u*::variant instead of std::variant).
>>>
>>
>> Yep, that's probably not ideal for most source generating use cases.
>>
>>
>>> (b) It will also display default template arguments, e.g.
>>> template <T = void>
>>> class MyClass
>>> is printed as MyClass (I think) in getFullyQualifiedName, while
>>> getAsString() will use MyClass<void>.
>>>
>>
>> That seems better/correct - did CLIF actually want/rely on/benefit from
>> the old behavior of only printing the template name?
>>
>
> I did check to replace all of the getFullyQualifiedName in CLIF with the
> new version: it worked like a charm. The error messages are just changed
> accordingly (so they will have longer types).
> And the generated code was also less verbose.
>
>>
>>
>>> (c) the example of the nested template argument above.
>>>
>>
>> Which seems just wrong/inconsistent/not-behaving-to-spec to me - I don't
>> imagine any caller actually wanted that behavior?
>>
>
> I agree.
>
>>
>>
>>>
>>> At the end,what matters is that getAsString() is semantically always
>>> correct, even if it can be overly verbose.
>>>
>>
>> The presence of inline namespaces is about the only bit I'd find a touch
>> questionable. (Hopefully Sam can chime in on some of that)
>>
>
> Me too, I would be curious if it is easy to remove.
>
>>
>>
>>> I tried to fix getFullyQualifiedName, but I do not understand its code.
>>>
>>> 3. When the goal we want to reach has been decided, there is also the
>>> question on how to transition from the current state to the next state, and
>>> who can help with that. To be way clearer, I won't be able to fix all of
>>> Google internal usage of this function (I am not at all working on Clang,
>>> Clif, or any tools, I just hit the bug and decided to look into it, and it
>>> happened that I found a fix). Thus, if we can do the changes using
>>> iterative steps, such as
>>> (a) add the new "::" prefix configuration,
>>> (b) Deprecate/document the fact that getFullyQualifiedName has a bug,
>>> and people should move to use qual_type.getAsString(Policy) instead, using
>>> "FullyQualifiedName" and "GlobalScopeQualifiedName". We can for example
>>> provide :
>>>
>>
>> It'd still fall to one of us Googlers to clean this all up inside Google
>> - since we build with -Werror, it's not like folks'll just get soft
>> encouragement to migrate away from the API, either the warning will be on
>> and their build will be broken (in which case we'll probably pick it up
>> when we integrate changes from upstream & have to fix it to complete that
>> integration) or no signal, and it'll break when the function is finally
>> removed.
>>
>>
>>> std::string GetFullyQualifiedCanonicalType(QualType qual_type, const
>>> ASTContext &ast,)
>>>   clang::PrintingPolicy print_policy(ast.GetASTContext().getLangOpts());
>>>   print_policy.FullyQualifiedName = 1;
>>>   print_policy.SuppressScope = 0;
>>>   print_policy.PrintCanonicalTypes = 1;
>>>   print_policy.GlobalScopeQualifiedName = 1;
>>>   QualType canonical_type = qual_type.getCanonicalType();
>>>   return canonical_type.getAsString(print_policy);
>>> }
>>> (c) Then, people can upgrade asynchronously, maybe someone will be
>>> unhappy with this behavior and will suggest something else, etc.
>>>
>>> If we just blindly delete it, it means for example that we have to fix
>>> all usages in Google to be able to upgrade Clang.  It may be the approach
>>> that is decided we should follow, but I just wanted to make it clear that I
>>> will not be able to do that job^^ While, having an incremental fix in
>>> Clang, and then fix Clif is doable as it does not imply to fix all calls in
>>> one go.
>>>
>>> I just wanted to develop these points.
>>>
>>
>> Sure enough - appreciate the awareness of the cost to external clients,
>> to be sure.
>>
>> - Dave
>>
>>
>>>
>>> Thanks!
>>>
>>> On Tue, May 12, 2020 at 7:59 PM David Blaikie <dblaikie at gmail.com>
>>> wrote:
>>>
>>>> +Mr. Smith for visibility.
>>>>
>>>> I'm /guessing/ the right path might be to change the implementation of
>>>> getFullyQualifiedName to use the type printing/pretty printer approach with
>>>> the extra feature you're suggesting. That way all users get the desired
>>>> behavior
>>>>
>>>> +Sam McCall <sammccall at google.com>  who (if I understand correctly)
>>>> has a lot to do with the Clang Tooling work - looks like Google's got a
>>>> bunch of uses of this function (getFullyQualifiedName) internally in clang
>>>> tools (I wonder why that's the case when there are still zero external
>>>> callers - is that OK? Or are external users doing something different
>>>> (better? worse?) to answer this question - or the tooling uses in LLVM
>>>> proper just don't have the same needs?). So probably best not to leave a
>>>> buggy implementation lying around - either deleting it, or fixing it.
>>>>
>>>> On Mon, May 11, 2020 at 11:28 PM Jean-Baptiste Lespiau via cfe-commits <
>>>> cfe-commits at lists.llvm.org> wrote:
>>>>
>>>>> Hi,
>>>>>
>>>>> *Context and history:*
>>>>>
>>>>> I have found a bug in CLIF <https://github.com/google/clif>, which
>>>>> does not correctly fully qualify templated names when they are nested, e.g.
>>>>>
>>>>> ::tensorfn::Nested< ::std::variant<Tensor, DeviceTensor> >
>>>>>
>>>>> should have been:
>>>>>
>>>>> ::tensorfn::Nested<
>>>>> ::std::variant<::tensorflow::Tensor,::tensorfn::DeviceTensor> >
>>>>>
>>>>> I tracked it down to
>>>>> https://github.com/google/clif/blob/master/clif/backend/matcher.cc#L172
>>>>> which calls
>>>>>
>>>>> https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp
>>>>> and the error is exactly at the line, but I could not really
>>>>> understand why.
>>>>>
>>>>> https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp#L457
>>>>>
>>>>> Historically, it has been added by the developers of CLIF
>>>>> (including Sterling Augustine)
>>>>>
>>>>> https://github.com/llvm/llvm-project/commit/0dd191a5c4bf27cc8a2b6033436b00f0cbdc4ce7
>>>>> .
>>>>> They explained to me, that they pushed this to Clang hoping it would
>>>>> be used by other tools and maintained by the community, but that it kind of
>>>>> failed at that, and it (i.e. QualTypeName.pp) is not much used, and not
>>>>> much maintained.
>>>>>
>>>>> I was not able to understand this file to provide a fix. On the other
>>>>> side, it was easy to understand TypePrinter.cpp and PrettyPrinter.cpp, so I
>>>>> tried extending it to fit my need.
>>>>>
>>>>> *Suggestion*
>>>>>
>>>>>  As I wanted fully qualified types (it's usually more convenient for
>>>>> tools generating code), to prevent some complex errors), I added ~10 lines
>>>>> that add an option to prepend "::" to qualified types (see the patch).
>>>>>
>>>>> In practice, it is still a different display at what QualTypeNames.cpp
>>>>> was doing, as, for example, it displays
>>>>>
>>>>> ::tensorfn::Nested<::std*::__u*::variant<tensorflow::Tensor,
>>>>> ::tensorfn::DeviceTensor>>
>>>>>
>>>>> but I was able to solve my issues. More generally, it is more verbose,
>>>>> as it displays the exact underlying type, including default parameter types
>>>>> in template arguments. So it's verbose, but it's correct (what is best?^^).
>>>>>
>>>>> I am contacting you, so we can discuss:
>>>>>
>>>>> - Whether QualTypeNames.cpp
>>>>> <https://github.com/llvm-mirror/clang/blob/master/lib/AST/QualTypeNames.cpp> is
>>>>> something useful for the Clang project, whether you think we should fix the
>>>>> bug I have found (but I cannot, as I do not understand it), or whether we
>>>>> should deprecate it, or modify the current printing mechanism
>>>>> (TypePrinter.cpp and PrettyPrinter.cpp) to add more options to tune the
>>>>> display in ways people may want to.
>>>>> - Whether adding the CL I have attached is positive, and if yes, what
>>>>> should be done in addition to that (does it need tests? Are there more
>>>>> types that we may want to prepend "::" to its fully qualified name?).
>>>>>
>>>>> Thanks!
>>>>>
>>>>> _______________________________________________
>>>>> cfe-commits mailing list
>>>>> cfe-commits at lists.llvm.org
>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20200523/d246029f/attachment-0001.html>