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

Tue Jun 2 00:37:42 PDT 2020

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/367650d0/attachment-0001.html>