Skip to content

Don't report CLS compliance warnings for attributes on inacessible types #68527

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 2 commits into
base: main
Choose a base branch
from
Open

Don't report CLS compliance warnings for attributes on inacessible types #68527

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5b29940
Don't report CLS compliance warnings for attributes on inacessible types
jkoritzinsky Jun 9, 2023
af0fa19
Update src/Compilers/CSharp/Test/Symbol/Symbols/Source/ClsComplianceT…
jkoritzinsky Jun 12, 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
8 changes: 2 additions & 6 deletions src/Compilers/CSharp/Portable/Compiler/ClsComplianceChecker.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,8 @@ private bool VisitTypeOrMember(Symbol symbol, Compliance compliance)
{
CheckMemberDistinctness((NamedTypeSymbol)symbol);
}

CheckForAttributeWithArrayArgument(symbol);
}
else if (GetDeclaredOrInheritedCompliance(symbol.ContainingAssembly) == Compliance.DeclaredTrue && IsTrue(GetInheritedCompliance(symbol)))
{
Expand All @@ -461,12 +463,6 @@ private bool VisitTypeOrMember(Symbol symbol, Compliance compliance)
return false; // Don't cascade from this failure.
}

if (isCompliant)
{
// Independent of accessibility.
Copy link
Contributor

Choose a reason for hiding this comment

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

// Independent of accessibility.

It looks to me that the behavior was very intentional. Usually attributes are examined via reflection, therefore, it feels like accessibility of the target should make no difference.

Copy link
Member Author

Choose a reason for hiding this comment

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

If they're examined through reflection, then the array arguments would be CLS-Compliant values (single-dimensional arrays starting at a 0 index). CLS Compliance for attributes seems to be focused around being able to represent the attribute as an attribute on a given member in any CLS-Compliant language (i.e. being able to present a VB member definition in Intellisense or documentation for a member defined initially in C#).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is the rule that is checked here: "CLS Rule 34: The CLS only allows a subset of the encodings of custom attributes. The
only types that shall appear in these encodings are (see Partition IV): System.Type,
System.String, System.Char, System.Boolean, System.Byte, System.Int16, System.Int32,
System.Int64, System.Single, System.Double, and any enumeration type based on a CLScompliant
base integer type."
Arrays in general are not on the list.

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 feel like CLS Rule 1 also applies here:

CLS Rule 1: CLS rules apply only to those parts of a type that are accessible or visible outside
of the defining assembly.
[Note:
CLS (consumer): no impact.
CLS (extender): when checking CLS compliance at compile time, be sure to apply the rules
only to information that will be exported outside the assembly.
CLS (framework): CLS rules do not apply to internal implementation within an assembly. A
type is CLS-compliant if all its publicly accessible parts (those classes, interfaces, methods,
fields, properties, and events that are available to code executing in another assembly) either
 have signatures composed only of CLS-compliant types, or
 are specifically marked as not CLS-compliant. end note]

Internal/private/file-scoped members are not accessible or visible outside of the defining assembly, so further CLS rules should not be applied. If reflection-access is considered to make members accessible, then technically all code should be checked for CLS-compliance.

Copy link
Contributor

Choose a reason for hiding this comment

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

So far I do not see a good justification for this "wide" change. If we were to say that file types are not subject to CLS compliance and make a specific change for that, that would be a different thing.

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

On accessible types, all Microsoft documentation tools will generate code snippets for a member definition in multiple languages, including the attribute definitions on that member. I believe the clauses on attributes were for that case.

I'd be fine taking a smaller change, but I would rather that the alternative change would be as follows:

If CLSCompliant(false) is applied to a member or a containing type that is not exposed externally, then it should not emit a warning if the type or any nested member has an attribute with array arguments.

I see no good scenario where there should be a warning about CLS Compliance, and the explicit gesture to say "this member is not CLS Compliant" would issue a warning saying "CLS Compliance will not be checked on this member, so this attribute is useless" when it is explicitly added to suppress a CLS Compliance warning.

Copy link
Contributor

Choose a reason for hiding this comment

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

If CLSCompliant(false) is applied to a member or a containing type that is not exposed externally, then it should not emit a warning if the type or any nested member has an attribute with array arguments.

It looks like you are describing the current behavior.

It appears that originally Roslyn had the attribute check implemented in the way it is implemented in this PR. Then, we explicitly changed that behavior back to accessibility independent behavior. Why would we want to flip back again after that many years? Apparently we had a good enough reason to end up where we are today with respect to the check.

Copy link
Member Author

Choose a reason for hiding this comment

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

If CLSCompliant(false) is applied to a member or a containing type that is not exposed externally, then it should not emit a warning if the type or any nested member has an attribute with array arguments.

It looks like you are describing the current behavior.

No, the warning I'm referring to there is the "CLS compliance checking will not be performed on 'Member' because it is not visible from outside this assembly" warning. Basically if I ever get the "Arrays as attribute arguments is not CLS-compliant" warning and I add a CLSCompliant(false) attribute, I should never get a "CLS compliance checking will not be performed on 'Member' because it is not visible from outside this assembly" on the CLSCompliant(false) attribute.

I can't see any good reason to get a "CLS compliance checking will not be performed on 'Member' because it is not visible from outside this assembly" warning from a CLSCompliant(false) attribute if I'm getting a CLS compliance warning without it.

I'd rather fix this one check to be consistent with every other CLS Compliance check in the system than update the "CLS compliance checking will not be performed on 'Member' because it is not visible from outside this assembly" warning to detect this nested case, but I'd also be okay with changing the warning on CLSCompliant(false) to detect this case and not emit the "compliance checking will not be performed" warning as it's clearly being performed in this case to some extent.

It appears that originally Roslyn had the attribute check implemented in the way it is implemented in this PR. Then, we explicitly changed that behavior back to accessibility independent behavior. Why would we want to flip back again after that many years? Apparently we had a good enough reason to end up where we are today with respect to the check.

From what I found and what I got when I spoke to a former member of the Roslyn team was that this check likely fell into the bucket of "get the regression tests from the native compiler test suite passing so we can ship Roslyn". Since generally the .NET product is one of few areas that cares about CLS Compliance and array arguments on attributes is pretty rare (especially since the BCL did not include any attributes with array arguments until recently), I'm not amazed that this never came up for discussion again.

This is coming up now because we're writing code that using UnmanagedCallersOnly attributes with a non-default calling convention in product code (not test code, where we don't set CLSCompliant(true)). In this particular case, we're source-generating it, so we can't even easily add pragmas without updating the generator, which is one of the reasons I filed the issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

It sounds like you are talking about two independent issues, even though when combined, they are making live even harder. I recommend having two separate issues for each. In the issues, clearly describe what changes you are proposing and why. Let's wait for triage to make the decision what path should be taken.

Copy link
Member Author

@jkoritzinsky jkoritzinsky Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Filed #68560 for the compounding issue. I wouldn't exactly consider them as separate issues as the warning on CLSCompliant(false) (the CS3019 warning) is correct per CLS Rule 1 above and would be accurate if a warning was not reported here, but I opened the issue anyway to help triage.

CheckForAttributeWithArrayArgument(symbol);
}

// These checks are independent of accessibility and compliance.
if (symbolKind == SymbolKind.NamedType)
{
Expand Down
11 changes: 3 additions & 8 deletions src/Compilers/CSharp/Test/Symbol/Symbols/Source/ClsComplianceTests.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3267,10 +3267,7 @@ public ArrayAttribute(object o) { }
class Test { }
";

CreateCompilation(source).VerifyDiagnostics(
// (11,2): warning CS3016: Arrays as attribute arguments is not CLS-compliant
// [Array(new int[] { 1 })]
Diagnostic(ErrorCode.WRN_CLS_ArrayArgumentToAttribute, "Array(new int[] { 1 })"));
CreateCompilation(source).VerifyDiagnostics();
}

[Fact]
Expand Down Expand Up @@ -3805,6 +3802,7 @@ public static int Main()

[Fact]
[WorkItem(741718, "http://vstfdevdiv:8080/DevDiv2/DevDiv/_workitems/edit/741718")]
[WorkItem(68526, "https://github.com/dotnet/roslyn/issues/68526")]
public void WRN_CLS_ArrayArgumentToAttribute_Inaccessible()
{
var source = @"
Expand All @@ -3826,10 +3824,7 @@ public static int Main()
}

";
CreateCompilation(source).VerifyDiagnostics(
// (4,2): warning CS3016: Arrays as attribute arguments is not CLS-compliant
// [My(new int[] { 1, 2 })]
Diagnostic(ErrorCode.WRN_CLS_ArrayArgumentToAttribute, "My(new int[] { 1, 2 })"));
CreateCompilation(source).VerifyDiagnostics();
}

[Fact]
Expand Down