Skip to content

[Clang] adjust caret placement for the suggested attribute location for enum class #168092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
+84 −26
Open

[Clang] adjust caret placement for the suggested attribute location for enum class #168092

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
16 commits
Select commit Hold shift + click to select a range
68dc4b9
[Clang] adjust caret placement for the suggested attribute location f…
a-tarasyuk Nov 14, 2025
5152768
derive enum attribute locations from EnumDecl instead of Lexer
a-tarasyuk Nov 14, 2025
9f93393
remove lambda
a-tarasyuk Nov 14, 2025
a495ee9
cleanup, add additional comment
a-tarasyuk Nov 14, 2025
7bc4552
Merge branch 'main' into fix/163224
a-tarasyuk Nov 14, 2025
303b91b
Merge branch 'main' into fix/163224
a-tarasyuk Nov 17, 2025
782923b
use explicit type
a-tarasyuk Nov 17, 2025
253af19
Merge branch 'fix/163224' of https://github.com/a-tarasyuk/llvm-proje…
a-tarasyuk Nov 17, 2025
f0f5dfd
Merge branch 'main' into fix/163224
a-tarasyuk Nov 17, 2025
1f565db
Merge branch 'main' into fix/163224
a-tarasyuk Nov 18, 2025
a394503
Merge branch 'main' into fix/163224
a-tarasyuk Nov 19, 2025
444bfc5
track enum key source range in EnumDecl
a-tarasyuk Nov 22, 2025
55ab1a4
Merge branch 'main' of https://github.com/llvm/llvm-project into fix/…
a-tarasyuk Nov 22, 2025
3b4f729
Merge branch 'fix/163224' of https://github.com/a-tarasyuk/llvm-proje…
a-tarasyuk Nov 22, 2025
adefaa1
Merge branch 'main' into fix/163224
a-tarasyuk Nov 23, 2025
62c9940
Merge branch 'main' into fix/163224
a-tarasyuk Nov 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -420,6 +420,8 @@ Improvements to Clang's diagnostics
or continue (#GH166013)
- Clang now emits a diagnostic in case `vector_size` or `ext_vector_type`
attributes are used with a negative size (#GH165463).
- Clang now provides correct caret placement when attributes appear before
`enum class` (#GH163224).

Improvements to Clang's time-trace
----------------------------------
Expand Down
41 changes: 21 additions & 20 deletions clang/lib/Parse/Parser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1100,30 +1100,31 @@ Parser::DeclGroupPtrTy Parser::ParseDeclOrFunctionDefInternal(
// C99 6.7.2.3p6: Handle "struct-or-union identifier;", "enum { X };"
// declaration-specifiers init-declarator-list[opt] ';'
if (Tok.is(tok::semi)) {
auto LengthOfTSTToken = [](DeclSpec::TST TKind) {
assert(DeclSpec::isDeclRep(TKind));
switch(TKind) {
case DeclSpec::TST_class:
return 5;
case DeclSpec::TST_struct:
return 6;
case DeclSpec::TST_union:
return 5;
case DeclSpec::TST_enum:
return 4;
case DeclSpec::TST_interface:
return 9;
default:
llvm_unreachable("we only expect to get the length of the class/struct/union/enum");
auto GetAdjustedAttrsLoc = [&]() {
auto TKind = DS.getTypeSpecType();
if (!DeclSpec::isDeclRep(TKind))
return SourceLocation();

if (TKind == DeclSpec::TST_enum) {
Copy link
Collaborator

@erichkeane erichkeane Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, the 'get location after the TST loc' logic works for all of these, right? With the exception being this is an enum class/enum struct?

Which, incidentially, doesn't actually work for enum class/enum struct? We potentially have OTHER tokens between the 'enum' and the 'class/struct' keywords (even if ill-formed potentially?), so in reality, if this is a scoped enum, we actually have to track down the class/struct token instead.

I find myself wondering if we should just have EnumDecl store the location of the class instead?

Alternatively, and probably better than all of this:
Since we are at the semi-colon, we actually already KNOW the location for the enum insertion point, right? It is either 'the name' of the enum, or, if there is no identifier, it is the LBrace/semicolon location, right? All of which should be accessible from ED. AND this should work whether or not it is scoped, right?

I find myself wondering if we SHOULD be smarter about the OTHER cases as well, rather than a sizeof-keyword-offset funny business, since that also doesn't necessarily work if there are other tokens in the way.

Copy link
Member Author

@a-tarasyuk a-tarasyuk Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we are at the semi-colon, we actually already KNOW the location for the enum insertion point, right? It is either 'the name' of the enum, or, if there is no identifier, it is the LBrace/semicolon location, right?

@erichkeane Thanks for the feedback. I had considered using the identifier or lbrace location, but I wasn’t sure whether placing the attribute directly before the name or the brace was acceptable.

Copy link
Collaborator

@erichkeane erichkeane Nov 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I THINK it is appropriate (identifier, lbrace, or semicolon)? I'm curious what @AaronBallman has to say.

Regarding the rest of the DeclSpec kinds, we could probably leave those alone + a fixme to 'get good' here.

Copy link
Member Author

@a-tarasyuk a-tarasyuk Nov 18, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@AaronBallman, whenever you have time, could you please review these changes?

Copy link
Collaborator

@AaronBallman AaronBallman Nov 19, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My reading of the grammar is that the attribute can go after the key (so either enum or enum class|struct), which I think is easier to reason about than what it can go before.

Because "before" means more logic; there could be an optional nested name specifier. e.g., https://godbolt.org/z/eMo5T8r53 or there could be no name but an enum base, e.g., https://godbolt.org/z/jbqjnrn3d and so on.

So I think what makes the most sense is for EnumDecl to track a source range for the enum key. So it'll either be a single token (enum) or it will be all of the tokens in the key (enum class). Then we can use the end of that range. Then the logic works regardless of whether the enum is scoped or not.

WDYT?

const auto *ED = dyn_cast_or_null<EnumDecl>(DS.getRepAsDecl());
if (ED) {
if (ED->isScoped() && ED->getIdentifier())
return ED->getLocation();

const auto Begin = ED->getBraceRange().getBegin();
if (Begin.isValid())
return Begin;
}
}

const auto &Policy = Actions.getASTContext().getPrintingPolicy();
unsigned Offset =
StringRef(DeclSpec::getSpecifierName(TKind, Policy)).size();
return DS.getTypeSpecTypeLoc().getLocWithOffset(Offset);
};

// Suggest correct location to fix '[[attrib]] struct' to 'struct [[attrib]]'
SourceLocation CorrectLocationForAttributes =
DeclSpec::isDeclRep(DS.getTypeSpecType())
? DS.getTypeSpecTypeLoc().getLocWithOffset(
LengthOfTSTToken(DS.getTypeSpecType()))
: SourceLocation();
SourceLocation CorrectLocationForAttributes = GetAdjustedAttrsLoc();
ProhibitAttributes(Attrs, CorrectLocationForAttributes);
ConsumeToken();
RecordDecl *AnonRecord = nullptr;
Expand Down
37 changes: 37 additions & 0 deletions clang/test/FixIt/fixit-cxx0x-attributes.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
// RUN: %clang_cc1 -fsyntax-only -verify %s
// RUN: not %clang_cc1 -fsyntax-only -fdiagnostics-parseable-fixits -fno-diagnostics-show-line-numbers %s 2>&1 | FileCheck %s -strict-whitespace

[[nodiscard]] enum class E1 { };
// expected-error@-1 {{misplaced attributes; expected attributes here}}
// CHECK: {{^}}{{\[\[}}nodiscard]] enum class E1 { };
// CHECK: {{^}}~~~~~~~~~~~~~ ^
// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:1-[[@LINE-4]]:15}:""
// CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:26-[[@LINE-5]]:26}:"{{\[\[}}nodiscard]]"

[[nodiscard]] enum struct E2 { };
// expected-error@-1 {{misplaced attributes; expected attributes here}}
// CHECK: {{^}}{{\[\[}}nodiscard]] enum struct E2 { };
// CHECK: {{^}}~~~~~~~~~~~~~ ^
// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:1-[[@LINE-4]]:15}:""
// CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:27-[[@LINE-5]]:27}:"{{\[\[}}nodiscard]]"

[[nodiscard]] enum class E3 { };
// expected-error@-1 {{misplaced attributes; expected attributes here}}
// CHECK: {{^}}{{\[\[}}nodiscard]] enum class E3 { };
// CHECK: {{^}}~~~~~~~~~~~~~ ^
// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:1-[[@LINE-4]]:15}:""
// CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:35-[[@LINE-5]]:35}:"{{\[\[}}nodiscard]]"

[[nodiscard]] enum /*comment*/ class E4 { };
// expected-error@-1 {{misplaced attributes; expected attributes here}}
// CHECK: {{^}}{{\[\[}}nodiscard]] enum /*comment*/ class E4 { };
// CHECK: {{^}}~~~~~~~~~~~~~ ^
// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:1-[[@LINE-4]]:15}:""
// CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:39-[[@LINE-5]]:39}:"{{\[\[}}nodiscard]]"

[[nodiscard]] enum { A = 0 };
// expected-error@-1 {{misplaced attributes; expected attributes here}}
// CHECK: {{^}}{{\[\[}}nodiscard]] enum { A = 0 };
// CHECK: {{^}}~~~~~~~~~~~~~ ^
// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:1-[[@LINE-4]]:15}:""
// CHECK: fix-it:"{{.*}}":{[[@LINE-5]]:20-[[@LINE-5]]:20}:"{{\[\[}}nodiscard]]"
Loading