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

Sam McCall via cfe-commits cfe-commits at lists.llvm.org
Fri May 29 07:05:56 PDT 2020 

Sorry about ignoring this, I didn't really have an opinion - clang is
fairly full of helper functions that don't quite do what you expect.
This does look like a bug though.

Generally for printing types, PrettyPrinter is the way to go. So I'm
supportive of any of:
 - adding the option to explicitly root names at the global namespace
 - modifying CLIF to use a PrintingPolicy directly
 - modifying getFullyQualifiedName() to do something roughly equivalent but
less buggy using the pretty-printer
 - deprecating getFullyQualifiedName()

> The presence of inline namespaces is about the only bit I'd find a touch
questionable
Does SuppressUnwrittenScope do what you want?

I did miss the patch attached to this, we do apparently have docs
<https://llvm.org/docs/Contributing.html#how-to-submit-a-patch> that say
mailing patch files to the *-commits@ mailing lists is a good way to get
them reviewed, but IMO those docs are out of date - Phabricator is the way
to go. Feel free to send it to me.
It will need a test, I think
llvm-project/clang/unittests/AST/DeclPrinterTest.cpp is probably the best
place to add one.

On Sat, May 23, 2020 at 10:22 PM Jean-Baptiste Lespiau <jblespiau at google.com>
wrote:

> 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/20200529/477719ef/attachment-0001.html>

More information about the cfe-commits mailing list