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

David Blaikie via cfe-commits cfe-commits at lists.llvm.org
Tue Jun 2 10:47:08 PDT 2020 

Ah, thanks for the context/update!

On Tue, Jun 2, 2020 at 12:37 AM Jean-Baptiste Lespiau <jblespiau at google.com>
wrote:

> Yes it was: https://reviews.llvm.org/D80800#2065643
>
> Thanks to Sam, I understand the situation better, but I have been slowed
> down quite a lot by the fact that
> (a) the logic to print types is duplicated both in TypePrinter.cpp and
> NamedDecl.{h, cpp}. There are 2 different code-paths that are printing the
> fully qualified namespaces. Thus, it probably means that to implement this
> feature, we need to do it for both, and test it for both.
> (a) there are lacking tests, so I have to add tests that should have been
> added a long time ago (thus, I have to understand the Matcher API, to be
> able to get back a specific qualified, to be able to retrieve it and print
> it, to check the output).
>
> I did spend some time during the weekend (as I am doing this in addition
> to my normal work, as this is not necessary for my project). So given the
> amount of work required, I am also looking if I cannot fix the bug in CLIF
> without such change in Clang, that, I must admit, will be difficult for me
> to get. Initially, I thought it would be at most a 10h fix, and now it's
> eating me up, so maybe I should stop for my own sake :P
>
> On Tue, Jun 2, 2020 at 2:47 AM David Blaikie <dblaikie at gmail.com> wrote:
>
>> Did this end up moving to a review thread? Could someone include a
>> link to that phab review, if it did.
>>
>> Otherwise - I'd still be curious about the answer to Sam's question
>> about SuppressUnwrittenScope & see if the inline namespaces could be
>> avoided & then going for the "modifying getFullyQualifiedName() to do
>> something roughly equivalent but less buggy using the pretty-printer"
>>
>> On Fri, May 29, 2020 at 7:06 AM Sam McCall <sammccall at google.com> wrote:
>> >
>> > 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 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  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, 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 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/20200602/b5ee0ee5/attachment-0001.html>

More information about the cfe-commits mailing list