[PATCH] D59692: [ASTImporter] Fix name conflict handling with different strategies

Mon Aug 26 06:49:38 PDT 2019

martong added a comment.

@shafik I have updated the patch with ODR handling strategies as per our discusson.

For the record I copy our discussion here:

On Sat, Aug 24, 2019 at 12:50 AM Shafik Yaghmour <yaghmour.shafik at gmail.com> wrote:

> Apologies, I missed this earlier!
> 
> On Wed, Aug 21, 2019 at 2:44 AM Gábor Márton <martongabesz at gmail.com> wrote:
>  >
>  > Hi Shafik,
>  >
>  > > Right now you will end up w/ an arbitrary one of them but we do want to support
>  > a way to choose between them eventually.
>  >
>  > Actually, right now (if the patch is not merged) we end up having both of them in the AST. Some parts of the AST reference the existing definition, while some other parts reference the new definition. Also, the regular lookup will find both definitions.
>  >
>  > If the patch is merged then only the first (the existing) definition is kept and an error is reported.
>  >
>  > > AFAICT this would prevent such a solution. At least that is how the
>  > new test case for RecordDecl make it appear
>  >
>  > Yes, you will never be able to remove an existing node from the AST, so I don't think an either-or choosing mechanism is feasible. But you may be able to decide whether you want to add the new and conflicting node. And you may be able to use a different name for the new conflicting node. This may help clients to see which node they are using.
>  > I want to create an additional patch which builds on this patch. Here is the essence of what I'd like to have:
>  >   if (!ConflictingDecls.empty()) {
>  >     Expected<DeclarationName> Resolution = Importer.HandleNameConflict(
>  >         Name, DC, Decl::IDNS_Member, ConflictingDecls.data(),
>  >         ConflictingDecls.size());
>  >     if (Resolution)
>  >       Name = Resolution.get();
>  >     else
>  >       return Resolution.takeError();
>  >   }
>  > Consider the "else" branch. I'd like to have such an "else" branch everywhere. The point is to use the result of HandleNameConflict (if it is set to something). This way it is possible to create any kind of ODR handling by overwriting HandleNameConflict in the descendant classes.
>  >
>  > We identified 3 possible strategies so far:
>  >
>  > Conservative. In case of name conflict propagate the error. This should be the default behavior.
>  > Liberal. In case of name conflict create a new node with the same name and do not propagate the error.
>  > Rename. In case of name conflict create a new node with a different name (e.g. with a prefix of the TU's name). Do not propagate the error.
>  >
>  >
>  > If we add a new conflicting node beside the exiting one, then some clients of the AST which rely on lookup will be confused. The CTU client does not rely on lookup so that is not really a problem there, but I don't know what this could cause with LLDB. Perhaps the renaming strategy could work there too.
>  > The Conservative and Liberal strategies are very easy to implement, and I am going to create patches for that if we have consensus.
> 
> We know currently we do have cases where we have ODR violations w/
>  RecordDecl due to use of an opaque struct in the API headers and a
>  concrete instance internally e.g.:
> 
> //opaque
>  struct A {
> 
>   char buf[16];
> 
> };
> 
> //concrete
>  struct A {
> 
>   double d;
>   int64_t x;
> 
> };
> 
> and we don't want this to become an error.
> 
> I think we would at least one the choice of Conservative or Liberal to
>  be configurable and maybe LLDB would default to Liberal. This would
>  enable us to keep the status quo and not break existing cases any
>  worse than they already are.
> 
> I would prefer that would be done in this PR since I don't want to be
>  in a situation where we branch internally and we have this change but
>  not the follow-up change.
> 
>> >  I don't see how like you comment says this does not effect CXXRecordDecl
>  >
>  > In my comment I do not say that CXXRecordDecls are exceptions from the general way of handling ODR violations.
>  > The comment is about that we shall not report ODR errors if we are not 100% sure that we face one.
>  > So we should report an ODR error only if the found Decl and the newly imported Decl have the same kind.
>  > I.e. both are CXXRecordDecls.
>  > For example, let's assume we import a CXXRecordDecl and we find an existing ClassTemplateDecl with the very same Name.
>  > Then we should not report an ODR violation.
> 
> Thank you for the clarification, I misunderstood the comment, now it
>  makes more sense.
> 
>> Thanks,
>  > Gabor
>  >
>  >
>  > On Mon, Aug 19, 2019 at 5:46 PM Shafik Yaghmour <yaghmour.shafik at gmail.com> wrote:
>  >>
>  >> Have a nice vacation :-)
>  >>
>  >> On Mon, Aug 19, 2019 at 8:40 AM Gábor Márton <martongabesz at gmail.com> wrote:
>  >> >
>  >> > Hi Shafik,
>  >> >
>  >> > I'll have an answer for you on Wednesday, I'm on vacation until then.
>  >> >
>  >> > Thanks,
>  >> > Gábor
>  >> >
>  >> > On Sat, 17 Aug 2019, 04:30 Shafik Yaghmour, <yaghmour.shafik at gmail.com> wrote:
>  >> >>
>  >> >> Hello Gábor,
>  >> >>
>  >> >> I was looking at;
>  >> >>
>  >> >> https://reviews.llvm.org/D59692
>  >> >>
>  >> >> again and I appreciate you added the new tests.
>  >> >>
>  >> >> I don't know how I missed this before but we would probably like to
>  >> >> support ODR violations in some reasonable manner. For example we have
>  >> >> one API that is using an opaque struct for the public API but the
>  >> >> private API used a non-opaque definition.
>  >> >>
>  >> >> I would have to come up with some sort of test case which is hard to
>  >> >> do, or at least I did not have success last time I tried. Right now
>  >> >> you will end up w/ an arbitrary one of them but we do want to support
>  >> >> a way to choose between them eventually.
>  >> >>
>  >> >> AFAICT this would prevent such a solution. At least that is how the
>  >> >> new test case for RecordDecl make it appear and I don't see how like
>  >> >> you comment says this does not effect CXXRecordDecl.
>  >> >>
>  >> >> -Shafik

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D59692/new/

https://reviews.llvm.org/D59692