Don't report CLS compliance warnings for attributes on inacessible types #68527
Conversation
dotnet-issue-labeler
bot
added
the
untriaged
| @@ -461,12 +463,6 @@ private bool VisitTypeOrMember(Symbol symbol, Compliance compliance) | |||
| return false; // Don't cascade from this failure. | |||
| } | |||
|
|
|||
| if (isCompliant) | |||
| { | |||
| // Independent of accessibility. | |||
There was a problem hiding this comment.
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#).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
I'd really like this to ship. Winforms is using |
|
I just hit this again in dotnet/runtime#99970 |
Fixes #68526
Fixes #68560
Interestingly, the initial issue that prompted this fix in 2013 (linked in the changed
[WorkItem]attribute below, even asks why a diagnostic should be reported in this case anyway.This was the only case of a CLS Compliance warning that would be issued when a member is not visible outside the assembly other than the "this CLSCompliant attribute is meaningless".