[Clang] add fix-it hints for unknown attributes #141305

a-tarasyuk · 2025-05-23T23:56:06Z

This patch adds fix-it hints for unknown attribute names when Clang suggests a correction

llvmbot · 2025-05-23T23:56:40Z

@llvm/pr-subscribers-clang-modules

@llvm/pr-subscribers-clang

Author: Oleksandr T. (a-tarasyuk)

Changes

This patch adds fix-it hints for unknown attribute names when Clang suggests a correction

Full diff: https://github.com/llvm/llvm-project/pull/141305.diff

3 Files Affected:

(modified) clang/docs/ReleaseNotes.rst (+3)
(modified) clang/lib/Sema/SemaDeclAttr.cpp (+7-6)
(added) clang/test/FixIt/fixit-unknown-attributes.cpp (+26)

diff --git a/clang/docs/ReleaseNotes.rst b/clang/docs/ReleaseNotes.rst
index b93fa33acc2a0..02cca0bb357f9 100644
--- a/clang/docs/ReleaseNotes.rst
+++ b/clang/docs/ReleaseNotes.rst
@@ -592,6 +592,9 @@ Improvements to Clang's diagnostics
   trigger a ``'Blue' is deprecated`` warning, which can be turned off with
   ``-Wno-deprecated-switch-case``.
 
+- Clang now emits fix-it hints for unknown attributes when a spelling
+  correction is suggested.
+
 Improvements to Clang's time-trace
 ----------------------------------
 
diff --git a/clang/lib/Sema/SemaDeclAttr.cpp b/clang/lib/Sema/SemaDeclAttr.cpp
index 8ce51cc2882bf..05b8e7d5a75f0 100644
--- a/clang/lib/Sema/SemaDeclAttr.cpp
+++ b/clang/lib/Sema/SemaDeclAttr.cpp
@@ -7867,15 +7867,16 @@ void Sema::checkUnusedDeclAttributes(Declarator &D) {
 
 void Sema::DiagnoseUnknownAttribute(const ParsedAttr &AL) {
   std::string NormalizedFullName = '\'' + AL.getNormalizedFullName() + '\'';
+  SourceRange NR = AL.getNormalizedRange();
+  SourceLocation Loc = NR.getBegin();
+
   if (auto CorrectedFullName =
           AL.getCorrectedFullName(Context.getTargetInfo(), getLangOpts())) {
-    Diag(AL.getNormalizedRange().getBegin(),
-         diag::warn_unknown_attribute_ignored_suggestion)
-        << NormalizedFullName << *CorrectedFullName << AL.getNormalizedRange();
+    Diag(Loc, diag::warn_unknown_attribute_ignored_suggestion)
+        << NormalizedFullName << *CorrectedFullName
+        << FixItHint::CreateReplacement(NR, *CorrectedFullName) << NR;
   } else {
-    Diag(AL.getNormalizedRange().getBegin(),
-         diag::warn_unknown_attribute_ignored)
-        << NormalizedFullName << AL.getNormalizedRange();
+    Diag(Loc, diag::warn_unknown_attribute_ignored) << NormalizedFullName << NR;
   }
 }
 
diff --git a/clang/test/FixIt/fixit-unknown-attributes.cpp b/clang/test/FixIt/fixit-unknown-attributes.cpp
new file mode 100644
index 0000000000000..6b74650942d0a
--- /dev/null
+++ b/clang/test/FixIt/fixit-unknown-attributes.cpp
@@ -0,0 +1,26 @@
+// RUN: %clang_cc1 -Wunknown-attributes -fsyntax-only -verify %s
+// RUN: %clang_cc1 -Wunknown-attributes -fsyntax-only -fdiagnostics-parseable-fixits %s 2>&1 | FileCheck %s
+
+[[gmu::deprected]] // expected-warning {{unknown attribute 'gmu::deprected' ignored; did you mean 'gnu::deprecated'?}}
+int f1(void) {
+  return 0;
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:3-[[@LINE-4]]:17}:"gnu::deprecated"
+
+[[gmu::deprecated]] // expected-warning {{unknown attribute 'gmu::deprecated' ignored; did you mean 'gnu::deprecated'?}}
+int f2(void) {
+  return 0;
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:3-[[@LINE-4]]:18}:"gnu::deprecated"
+
+[[gnu::deprected]] // expected-warning {{unknown attribute 'gnu::deprected' ignored; did you mean 'gnu::deprecated'?}}
+int f3(void) {
+  return 0;
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:3-[[@LINE-4]]:17}:"gnu::deprecated"
+
+[[deprected]] // expected-warning {{unknown attribute 'deprected' ignored; did you mean 'deprecated'?}}
+int f4(void) {
+  return 0;
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:3-[[@LINE-4]]:12}:"deprecated"

zwuis · 2025-05-24T01:03:17Z

Can you handle this case [[using gnu : deprected]]? (or add tests)

AaronBallman · 2025-05-27T11:27:00Z

clang/test/FixIt/fixit-unknown-attributes.cpp

+int f5(void) {
+  return 0;
+}
+// CHECK: fix-it:"{{.*}}":{[[@LINE-4]]:9-[[@LINE-4]]:24}:"gnu::deprecated"


I think this shows a bug with the patch because this seems to be replacing [[using gnu: deprected]] with [[gnu::deprecated]]. While this is 1:1 in this example, consider something like: [[using gnu: deprected, noreturn]] where there are multiple attributes all sharing the same prefix.

I think using should be handled in a separate patch; for this PR, I'd recommend not attempting to fix this situation. But we may not have that information in Sema, so that may be more involved.

But we may not have that information in Sema, so that may be more involved.

I've checked — you're right. We should at least include the info if the attribute uses a common namespace

llvm-project/clang/lib/Parse/ParseDeclCXX.cpp

Line 4664 in 062353d

CommonScopeName = TryParseCXX11AttributeIdentifier(

llvm-project/clang/lib/Parse/ParseDeclCXX.cpp

Lines 4715 to 4723 in 062353d

if (CommonScopeName) {

if (ScopeName) {

Diag(ScopeLoc, diag::err_using_attribute_ns_conflict)

<< SourceRange(CommonScopeLoc);

} else {

ScopeName = CommonScopeName;

ScopeLoc = CommonScopeLoc;

}

}

I think using should be handled in a separate patch;

Sure, I'll look into it and prepare a new PR for this case.

AaronBallman · 2025-05-27T11:27:51Z

clang/lib/Sema/SemaDeclAttr.cpp

-    Diag(AL.getNormalizedRange().getBegin(),
-         diag::warn_unknown_attribute_ignored)
-        << NormalizedFullName << AL.getNormalizedRange();
+    Diag(Loc, diag::warn_unknown_attribute_ignored) << NormalizedFullName << NR;


There are more places where we emit a warn_unknown_attribute_ignored diagnostic; should any of those be handled as well? For example, target-specific attributes which are typos, etc.

@AaronBallman, thanks for the feedback. I replaced diag::warn_unknown_attribute_ignored with DiagnoseUnknownAttribute for known cases. Since this general message is used in many places, it might be worth reviewing all cases to see where it would be appropriate to use DiagnoseUnknownAttribute instead. If any additinal cases should be covered, I’d appreciate it if you could point them out — I’ll be happy to investigate., thanks.

The ones I'd look at replacing are:

ProhibitCXX11Attributes

ParseCXX11AttributeArgs

checkUnusedDeclAttributes

maybe CheckAttrTarget?

ActOnBaseSpecifier just looks odd to me; we diagnose an unknown attribute but if we knew about the attribute, we'd diagnose it anyway. Probably something we want to address separately?

ProcessStmtAttribute

erichkeane

I'm really in favor of this direction, this is great, thank you!

I have a few suggestions here (and if there is good reason, feel free to ignore the ones re return-type & setting invalid in particular), but this is pretty good for me.

erichkeane · 2025-05-28T13:43:03Z

clang/lib/Parse/ParseDecl.cpp

@@ -1842,8 +1842,7 @@ void Parser::ProhibitCXX11Attributes(ParsedAttributes &Attrs,
      continue;
    if (AL.getKind() == ParsedAttr::UnknownAttribute) {
      if (WarnOnUnknownAttrs)
-        Diag(AL.getLoc(), diag::warn_unknown_attribute_ignored)
-            << AL << AL.getRange();
+        Actions.DiagnoseUnknownAttribute(AL);


Curious that this is not setting it to invalid, but the branch below is. Is that a mistake?

I'll check, though it seems that unknown attributes should also be marked as invalid

erichkeane · 2025-05-28T13:44:48Z

clang/lib/Sema/SemaDeclAttr.cpp

@@ -7867,15 +7865,16 @@ void Sema::checkUnusedDeclAttributes(Declarator &D) {

 void Sema::DiagnoseUnknownAttribute(const ParsedAttr &AL) {
  std::string NormalizedFullName = '\'' + AL.getNormalizedFullName() + '\'';
+  SourceRange NR = AL.getNormalizedRange();


There are a few uses of this function, I wonder if we switched it to returning bool and taking by reference (instead of const-reference) if making this always return 'true' (such that the cases followed immediately by 'return true' could just return), AND having it set the Attribute as invalid would be valuable.

ALSO-ALSO: Could we change this to take AttributeCommonInfo instead? Or is there something that we need the full ParsedAttr for?

AND having it set the Attribute as invalid would be valuable.
Could we change this to take AttributeCommonInfo instead? Or is there something that we need the full ParsedAttr for?

@erichkeane Thanks for the feedback. In general, it’s possible to use AttributeCommonInfo, but I used ParsedAttr because all existing cases that handle unknown attributes rely on ParsedAttr. If we switch to AttributeCommonInfo, we lose the ability to mark the attribute as invalid, since setIsInvalid is only available on ParsedAttr.

@erichkeane we've a discussion about this case with @AaronBallman. Unless we plan a broader refactoring to properly support attribute groups, would it make sense to provide fixit only when there is no scope?

[deprecatd] // fix-it

To handle cases involving using correctly, it's necessary to at least preserve whether the scope was part of an attribute-using-prefix. WDYT?

Re: ParsedAttr/vs CommonInfo: Ack, that makes sense.

Re fixit only with no scope: I think that is somewhat sensible. I think it is at least an improvement.

You're right that we'd have to some pretty sizable changes to represent the 'scope' via using to the AST, and I don't have a great idea how that should look. We'd either have to do a 'normalization' for the group to make our 'fixit' work (that is, we end up converting usings into explicits or vice versa?), OR represent them.

That said, we COULD fixit just the identifier where it is, right? Since we already know the scope and are correcting within the scope, we can't just change the current 'identifier' to what we're suggesting as a fixit? IF we limited it that way, it would 'inherit' whatever sort of scoping was already there perhaps?

That said, we COULD fixit just the identifier where it is, right?

[[deprecatd]] - fixit

[[gmp::deprecatd]] - we could provide a fix-it, but since there's no difference between :: and a scope introduced via using, it might lead to incorrect suggestions in using cases.

[[using gmp: deprecatd, noreturn]] - not ok, because fixit will be gnu::deprecated

Since we already know the scope and are correcting within the scope, we can't just change the current 'identifier' to what we're suggesting as a fixit? IF we limited it that way, it would 'inherit' whatever sort of scoping was already there perhaps?

What I was thinking is that if we have information about the attribute-using prefix, we could avoid normalization when suggesting a typo correction (while still using the scope::attr format in the diagnostic message). Instead, we could provide two separate fix-its: one for the scope and one for the attribute name. Not sure if that approach makes sense.

What I was thinking is that if we have information about the attribute-using prefix, we could avoid normalization when suggesting a typo correction (while still using the scope::attr format in the diagnostic message). Instead, we could provide two separate fix-its: one for the scope and one for the attribute name. Not sure if that approach makes sense.

It DOES make sense. I was sorta hoping we could come up with a method to do the two separate on an individual identifier basis (so doing 1 level of correction at a scope, then a separate on on the attribute itself).

BUT this requires us to act separately on the scope so we cna do some sort of validation/fixit.

BUT this requires us to act separately on the scope so we cna do some sort of validation/fixit.

To proceed, I think the starting point should be to track whether the scope was explicitly written as part of a using-prefix. From what I understand, that might require introducing something like attrs groups — or is there a more suitable approach?

I think that isn't really necessary and we can just correct the scope earlier. If you look at Parser::ParseCXX11AttributeSpecifierInternal, we separately parse the 'using' from the inline-scope. I'm suggesting we do an ActOnCXX11AttributeScope where we actually do the correction to be a scope that we know.

That way, in the normal unknown-attributes code, we can treat the scope as 100% valid (since it has already been fixit'ed, or, just don't suggest if it isn't valid), and only suggest the name.

So we would be parsing:
[[using gnX::
-> ActOnCXX11AttributesScope : Diagnose: "What scope did you mean? could it have been gnu?"
deprecat
-> :Diagnose "what attribute in gnu did you mean, could it have been deprecated?"

[[gnX::
-> ActOnCXX11AttributesScope : Diagnose: "What scope did you mean? could it have been gnu?"
deprecat
-> :Diagnose "what attribute in gnu did you mean, could it have been deprecated?"

erichkeane · 2025-05-28T13:46:09Z

clang/lib/Sema/SemaStmtAttr.cpp

-                           ? (unsigned)diag::warn_unhandled_ms_attribute_ignored
-                           : (unsigned)diag::warn_unknown_attribute_ignored)
-        << A << A.getRange();
+    if (A.isRegularKeywordAttribute() || A.isDeclspecAttribute())


This 'if' is REALLY pushing our 'single-statement, no curleys' rule, and I think hits: However, braces should be used in cases where the omission of braces harm the readability and maintainability of the code.

So I'd ask for curleys on this + the else.

@erichkeane Sure, I'll change

a-tarasyuk requested review from erichkeane and AaronBallman May 23, 2025 23:56

llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels May 23, 2025

AaronBallman reviewed May 27, 2025

View reviewed changes

llvmbot added the clang:modules C++20 modules and Clang Header Modules label May 27, 2025

erichkeane reviewed May 28, 2025

View reviewed changes

a-tarasyuk marked this pull request as draft May 30, 2025 23:46

a-tarasyuk force-pushed the fix/fixit-unknown-attributes branch 13 times, most recently from db4a85b to 50e590f Compare May 31, 2025 18:12

a-tarasyuk removed the clang:modules C++20 modules and Clang Header Modules label May 31, 2025

a-tarasyuk force-pushed the fix/fixit-unknown-attributes branch from 50e590f to 9d2139c Compare May 31, 2025 20:28

[Clang] add fix-it hints for unknown attributes

77641f8

a-tarasyuk force-pushed the fix/fixit-unknown-attributes branch from 9d2139c to 77641f8 Compare May 31, 2025 22:26

	if (CommonScopeName) {
	if (ScopeName) {
	Diag(ScopeLoc, diag::err_using_attribute_ns_conflict)
	<< SourceRange(CommonScopeLoc);
	} else {
	ScopeName = CommonScopeName;
	ScopeLoc = CommonScopeLoc;
	}
	}

[Clang] add fix-it hints for unknown attributes #141305

Are you sure you want to change the base?

[Clang] add fix-it hints for unknown attributes #141305

Uh oh!

Conversation

a-tarasyuk commented May 23, 2025

Uh oh!

llvmbot commented May 23, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zwuis commented May 24, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

a-tarasyuk May 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

erichkeane left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

llvmbot commented May 23, 2025 •

edited

Loading

zwuis commented May 24, 2025 •

edited

Loading

a-tarasyuk May 27, 2025 •

edited

Loading