Skip to content
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
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
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 diagnosticId argument to the 'Experimental' attribute must be a valid identifier</value>
</data>
</root>
2 changes: 2 additions & 0 deletions src/Compilers/CSharp/Portable/Errors/ErrorCode.cs
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
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
9 changes: 9 additions & 0 deletions src/Compilers/CSharp/Portable/Symbols/Symbol_Attributes.cs
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,15 @@ 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

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

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.

{
Debug.Assert(arguments.AttributeSyntaxOpt is not null);
Copy link
Contributor

@cston cston Oct 18, 2023

Choose a reason for hiding this comment

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

Debug.Assert(arguments.AttributeSyntaxOpt is not null);

This is already asserted earlier. #Resolved

var attrArgumentLocation = arguments.Attribute.GetAttributeArgumentSyntaxLocation(parameterIndex: 0, arguments.AttributeSyntaxOpt);
arguments.Diagnostics.DiagnosticBag.Add(ErrorCode.ERR_InvalidExperimentalDiagID, attrArgumentLocation);
return;
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.

return

Shouldn't we still call DecodeWellKnownAttributeImpl below? #Closed

Copy link
Member Author

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

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

82 changes: 82 additions & 0 deletions src/Compilers/CSharp/Test/CommandLine/CommandLineTests.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14875,6 +14875,88 @@ public static void M() { }
var comp = CreateCompilation(src, options: TestOptions.DebugExe.WithGeneralDiagnosticOption(ReportDiagnostic.Suppress));
comp.VerifyDiagnostics();
}

[Fact]
public void ExperimentalWithWhitespaceDiagnosticID()
{
var dir = Temp.CreateDirectory();
var src = dir.CreateFile("test.cs").WriteAllText("""
C.M();

[System.Diagnostics.CodeAnalysis.Experimental(" ")]
public class C
{
public static void M() { }
}

namespace System.Diagnostics.CodeAnalysis
{
public sealed class ExperimentalAttribute : Attribute
{
public ExperimentalAttribute(string diagnosticId) { }
}
}
""");
var analyzerConfig = dir.CreateFile(".editorconfig").WriteAllText("""
[*.cs]
dotnet_diagnostic.CS9208.severity = warning
Copy link
Contributor

@AlekseyTs AlekseyTs Oct 19, 2023

Choose a reason for hiding this comment

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

Does this have any effect on the scenario? If yes, then consider including observations without this line present. If no, consider removing. #Closed

Copy link
Contributor

Choose a reason for hiding this comment

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

Or did you mean to attempt to suppress CS9211 instead?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, should have been CS9211. Thanks

""");
var cmd = CreateCSharpCompiler(null, dir.Path, new[] {
"/nologo",
"/t:exe",
"/preferreduilang:en",
"/analyzerconfig:" + analyzerConfig.Path,
src.Path });

Assert.Equal(analyzerConfig.Path, Assert.Single(cmd.Arguments.AnalyzerConfigPaths));

var outWriter = new StringWriter(CultureInfo.InvariantCulture);
var exitCode = cmd.Run(outWriter);
Assert.Equal(1, exitCode);
Assert.StartsWith("test.cs(3,2): error CS9208: The diagnosticId argument to the 'Experimental' attribute must be a valid identifier",
outWriter.ToString());
}

[Fact]
public void ExperimentalWithValidDiagnosticID()
{
var dir = Temp.CreateDirectory();
var src = dir.CreateFile("test.cs").WriteAllText("""
C.M();

[System.Diagnostics.CodeAnalysis.Experimental("DiagID")]
public class C
{
public static void M() { }
}

namespace System.Diagnostics.CodeAnalysis
{
public sealed class ExperimentalAttribute : Attribute
{
public ExperimentalAttribute(string diagnosticId) { }
}
}
""");
var analyzerConfig = dir.CreateFile(".editorconfig").WriteAllText("""
[*.cs]
dotnet_diagnostic.DiagID.severity = warning
""");
var cmd = CreateCSharpCompiler(null, dir.Path, new[] {
"/nologo",
"/t:exe",
"/preferreduilang:en",
"/analyzerconfig:" + analyzerConfig.Path,
src.Path });

Assert.Equal(analyzerConfig.Path, Assert.Single(cmd.Arguments.AnalyzerConfigPaths));

var outWriter = new StringWriter(CultureInfo.InvariantCulture);
var exitCode = cmd.Run(outWriter);
Assert.Equal(0, exitCode);
Assert.StartsWith("test.cs(1,1): warning DiagID: 'C' is for evaluation purposes only and is subject to change or removal in future updates. Suppress this diagnostic to proceed.",
outWriter.ToString());
}
}

[DiagnosticAnalyzer(LanguageNames.CSharp, LanguageNames.VisualBasic)]
Expand Down
Loading