Skip to content

Validate diagnostic ID in Experimental attribute #70397

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

Merged
merged 11 commits into from
Oct 20, 2023
Merged

Validate diagnostic ID in Experimental attribute #70397

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a7ff4a2
Validate diagnostic ID in Experimental attribute
Oct 16, 2023
c4534ef
fix test
Oct 17, 2023
75e7b28
Address feedback
Oct 18, 2023
3bdd89b
Merge remote-tracking branch 'dotnet/main' into exp-validation
Oct 18, 2023
6ad3134
Remove duplicate assertion
Oct 18, 2023
7a9753e
update test
Oct 19, 2023
a520b58
More tests
Oct 19, 2023
5d1c56d
one more
Oct 19, 2023
c4f5f8f
VB
Oct 19, 2023
caaf149
tweaks
Oct 20, 2023
43ea3e5
Avoid using AttributeData in PE symbols
Oct 20, 2023
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
3 changes: 3 additions & 0 deletions src/Compilers/CSharp/Portable/CSharpResources.resx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7809,4 +7809,7 @@ To remove the warning, you can use /reference instead (set the Embed Interop Typ
<data name="ERR_ExpectedInterpolatedString" xml:space="preserve">
<value>Expected interpolated string</value>
</data>
<data name="ERR_InvalidExperimentalDiagID" xml:space="preserve">
<value>The argument to the 'Experimental' attribute must be a valid identifier</value>
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

The argument

"The first argument"? The spec implies that there could be more than one argument and the restriction is for the first one. #Closed

</data>
</root>
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2275,6 +2275,8 @@ internal enum ErrorCode
ERR_InterceptorGlobalNamespace = 9206,
ERR_InterceptableMethodMustBeOrdinary = 9207,

ERR_InvalidExperimentalDiagID = 9208,

#endregion

// Note: you will need to do the following after adding warnings:
Expand Down
1 change: 1 addition & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorFacts.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2404,6 +2404,7 @@ internal static bool IsBuildOnlyDiagnostic(ErrorCode code)
case ErrorCode.WRN_Experimental:
case ErrorCode.ERR_ExpectedInterpolatedString:
case ErrorCode.ERR_InterceptorGlobalNamespace:
case ErrorCode.ERR_InvalidExperimentalDiagID:
return false;
default:
// NOTE: All error codes must be explicitly handled in this switch statement
Expand Down
7 changes: 7 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,13 @@ protected void DecodeWellKnownAttribute(ref DecodeWellKnownAttributeArguments<At
return;
}

if (arguments.Attribute.IsTargetAttribute(this, AttributeDescription.ExperimentalAttribute)
&& !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

&& !SyntaxFacts.IsValidIdentifier((string?)arguments.Attribute.CommonConstructorArguments[0].ValueInternal))

Similar refactoring suggestion as for VB #Closed

Copy link
Contributor

@AlekseyTs AlekseyTs Oct 20, 2023

Choose a reason for hiding this comment

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

On the second thought, never mind. It looks like we do want to call DecodeWellKnownAttributeImpl below.

{
arguments.Diagnostics.DiagnosticBag.Add(ErrorCode.ERR_InvalidExperimentalDiagID, arguments.AttributeSyntaxOpt.Location);
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 18, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

arguments.AttributeSyntaxOpt.Location

I think we have a helper that finds syntax for specific argument when there is one #Closed

return;
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 20, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

return

Shouldn't we still call DecodeWellKnownAttributeImpl below? #Closed

Copy link
Member Author

@jcouv jcouv Oct 20, 2023

Choose a reason for hiding this comment

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

I don't think so. The decoding of experimental attribute is done in EarlyDecodeWellKnownAttributes, before we get to DecodeWellKnownAttribute

Copy link
Contributor

@AlekseyTs AlekseyTs Oct 20, 2023

Choose a reason for hiding this comment

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

I don't think so. The decoding of experimental attribute is done in EarlyDecodeWellKnownAttributes, before we get to DecodeWellKnownAttribute

First, the code should be robust to future changes and it feels reasonable to me that the fact you are referring to, if it is a fact, won't hold in the future.

Second, I actually see decoding done in DecodeWellKnownAttributeImpl for a SourceModuleSymbol today, for example.

Copy link
Member Author

@jcouv jcouv Oct 20, 2023

Choose a reason for hiding this comment

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

Yes, you're right. The return broke things.

}

DecodeWellKnownAttributeImpl(ref arguments);
}
#nullable disable
Expand Down
5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.cs.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.de.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.es.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.fr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.it.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ja.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ko.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pl.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.pt-BR.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.ru.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.tr.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hans.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

5 changes: 5 additions & 0 deletions src/Compilers/CSharp/Portable/xlf/CSharpResources.zh-Hant.xlf
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading