Page MenuHomePhabricator

[modules] Fix ambiguous name lookup for enum constants from hidden submodules.
Changes PlannedPublic

Authored by vsapsai on Nov 30 2021, 5:08 PM.

Details

Summary

Fix errors like

clang/test/Modules/redefinition-c-tagtypes.m:36:10: error: reference to 'FST' is ambiguous
  return FST;
         ^
clang/test/Modules/redefinition-c-tagtypes.m:24:3: note: candidate found by name lookup is 'FST'
  FST = 22,
  ^
clang/test/Modules/Inputs/F.framework/PrivateHeaders/NS.h:7:3: note: candidate found by name lookup is 'FST'
  FST = 22,
  ^

The name lookup is ambiguous because we have 2 EnumConstantDecl for the
same identifier - one from hidden submodule, another non-modular from .m
file. One way to avoid ambiguity is for decls to have the same canonical
decl. But in this case the hidden decl is deserialized during lexing,
right before non-modular decl is created. So ASTDeclReader cannot do
anything useful in mergeMergeable(Mergeable<EnumConstantDecl>*) and
cannot make different decls have the same canonical decl.

The fix is roughly a follow-up to D31778. As a duplicate non-modular
EnumDecl is dropped, we are dropping its constants too and remove them
from the global name lookup. Another option was to avoid adding
EnumConstantDecl to IdResolver when we know they are parsed for
comparison only. But we still need that global name lookup for constants
referencing other constants from the same enum, like MinXOther = MinX.
So instead we remove the constants after ParseEnumBody is done.

rdar://82908206

Diff Detail

Unit TestsFailed

TimeTest
40 msx64 debian > Clang.Modules::ambiguous-anonymous-enum-lookup.cpp
Script: -- : 'RUN: at line 1'; rm -rf /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/test/Modules/Output/ambiguous-anonymous-enum-lookup.cpp.tmp

Event Timeline

vsapsai created this revision.Nov 30 2021, 5:08 PM
vsapsai requested review of this revision.Nov 30 2021, 5:08 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 30 2021, 5:08 PM
vsapsai added inline comments.Nov 30 2021, 5:14 PM
clang/lib/Sema/SemaDecl.cpp
16216–16223

Here is the early return the added comment refers to.

vsapsai added a subscriber: Restricted Project.Nov 30 2021, 5:14 PM

After more testing and thinking I've realized we are still not handling anonymous enums properly. On one hand anonymous EnumDecl aren't duplicates and we aren't making a hidden EnumDecl + EnumConstantDecl visible and don't have ambiguity problems. On the other hand, when we do make them visible through explicit import, we are hitting ambiguous references again.

Need to check the previous discussions and see if can bring ObjC/C closer to C++ in context of handling these duplicates. In ASTReaderDecl I was trying to keep ObjC/C support similar to C++ (modulo templates) and it turned out to be better for comprehension.

vsapsai updated this revision to Diff 391187.Dec 1 2021, 7:25 PM

Add a test case for referencing an anonymous enum constant in C++.

vsapsai updated this revision to Diff 391189.Dec 1 2021, 7:30 PM

Attempt to restore a previous commit.

As I was trying to replicate C++ behavior I've created a test case ambiguous-anonymous-enum-lookup.cpp. And it results in diagnostics

clang/test/Modules/Output/ambiguous-anonymous-enum-lookup.cpp.tmp/test.cpp:6:10: warning: ambiguous use of internal linkage declaration 'kAnonymousEnumValue' defined in multiple modules [-Wmodules-ambiguous-internal-linkage]
  return kAnonymousEnumValue;
         ^
clang/test/Modules/Output/ambiguous-anonymous-enum-lookup.cpp.tmp/include/textual.h:5:3: note: declared here
  kAnonymousEnumValue = 0,
  ^
clang/test/Modules/Output/ambiguous-anonymous-enum-lookup.cpp.tmp/include/textual.h:5:3: note: declared here in module 'Piecewise.InitiallyHidden'
  kAnonymousEnumValue = 0,
  ^

Is this warning correct or is it wrong and we have the same bug both in ObjC and C++? I feel I am biased (and bad at C++ linkage rules), so need a separate opinion on correctness.

Adding Michael who is infinitely better than me in C++.

vsapsai planned changes to this revision.Dec 13 2021, 2:17 PM

In discussions outside of this review the consensus is that "warning: ambiguous use of internal linkage declaration" is correct, so I won't change anything for C++. For Objective-C I still need to find a way to handle anonymous enums.