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

Tue May 12 14:19:36 PDT 2020

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/20200512/fea917dc/attachment.html>