Revert "do not emit diagnostic when attribute argument is internal type" #27774

smasher164 · 2022-09-08T21:04:16Z

The diagnostic emitted that this PR suppressed accurately identified a mismatch in the type arguments to TypeConverterAttribute. PR dotnet/runtime#75235 addresses that mismatch, and it's unclear if internal type arguments actually need to be excluded from the attribute rule.

Reverts #27648

…pe (#27648)" This reverts commit c533dcc.

ViktorHofer · 2022-09-08T22:02:07Z

PR dotnet/runtime#75235 addresses that mismatch, and it's unclear if internal type arguments actually need to be excluded from the attribute rule.

Do you know why this was the case in the old ApiCompat?

smasher164 · 2022-09-08T22:17:56Z

PR dotnet/runtime#75235 addresses that mismatch, and it's unclear if internal type arguments actually need to be excluded from the attribute rule.

Do you know why this was the case in the old ApiCompat?

After speaking with @ericstj, it was apparently due to the fact that the PublicOnlyCciFilter didn't even visit attribute arguments invisible outside the assembly. See https://github.com/dotnet/arcade/blob/e7ede87875f41a9b3df898ae08da5ebc96e24f56/src/Microsoft.Cci.Extensions/Filters/PublicOnlyCciFilter.cs#L72. I'm not sure why that was, but it has something to do with the fact that the Cci filters were used in code generation?

ericstj · 2022-09-09T17:09:51Z

I'm not sure why that was, but it has something to do with the fact that the Cci filters were used in code generation?

That’s my best guess. Otherwise GenAPI would emit code that didn’t compile. There wouldn’t be any override to tell GenAPI to exclude that one applied attribute - or include the internal type.

For API Compat I think it makes more sense to include here if the user is telling us that they think the Attribute should be tested. They have opted into treating uses of that attribute on public API as public.

ericstj · 2022-09-09T17:11:25Z

src/Tests/Microsoft.DotNet.ApiCompatibility.Tests/Rules/AttributesMustMatchTests.cs

 ",
 new CompatDifference[] {
    CompatDifference.CreateWithDefaultMetadata(DiagnosticIds.CannotChangeAttribute, "", DifferenceType.Changed, "T:CompatTests.First:[T:CompatTests.FooAttribute]")
 }


Since we desire this behavior can we add an explicit test for it? That should avoid having this discussion again in the future :)

To clarify, do you mean adding a test that asserts that changes to internal type arguments are caught?

Yeah. For instance add a test that might catch if we had an inconsistent namespace between the internal type argument on left and right. The test should prove that we look at the internal type argument and compare it, and have a comment around why.

Shouldn't we only validate internal members when this flag is set to true?

sdk/src/ApiCompat/Microsoft.DotNet.ApiCompatibility/ApiComparerSettings.cs

Lines 23 to 26 in 412df6f

/// <summary>

/// Determines if internal members should be validated.

/// </summary>

public readonly bool IncludeInternalSymbols;

I don't think so. That flag will only be set when people want to track their internal API and catch breaking changes in their internal API.

In this case this is not an internal API. It's an attribute on public API that a compiler is treating like public API. The concept of internal/public on attributes is not well defined -- the uses of attributes by design time tools are case-by-case reading of the metadata. The application of the attribute itself does not have a public/internal distinction. We have data here that shows there are cases where design time tools treat the application of the attribute as public even when it points at an internal type, so I believe the correct behavior here is to treat it as public.

The concept of internal/public on attributes is not well defined

Based on the previous behavior, right now, IncludeInternalSymbols applies to the attribute class as well. If we didn't want to do that, then I can remove that behavior as well.

We still filter out attributes which aren't visible outside of the assembly, even with this PR; but we don't filter out visible attributes which contain internal members anymore. That's the desired behavior, correct?

Yes, that is correct. The internal type arguments should still be considered, no matter what.

ericstj · 2022-09-13T21:59:49Z

src/Tests/Microsoft.DotNet.ApiCompatibility.Tests/Rules/AttributesMustMatchTests.cs

 ",
-new CompatDifference[] {}
+new CompatDifference[] {
+    CompatDifference.CreateWithDefaultMetadata(DiagnosticIds.CannotChangeAttribute, "", DifferenceType.Changed, "T:CompatTests.First:[T:CompatTests.FooAttribute]"),


Thank you for adding the test - you may want to also include some comments that explain why it's important to examine internal types.

ViktorHofer · 2022-09-21T19:23:29Z

@smasher164 do you want to get this into release/7.0?

smasher164 · 2022-09-21T20:44:51Z

@smasher164 do you want to get this into release/7.0?

Yes. I believe we decided that internal type arguments to a public attribute shouldn't be filtered out by typ.IsVisibleOutsideOfAssembly.

ViktorHofer · 2022-09-22T19:08:55Z

Cool. I was asking because we are past RC2 now and still getting changes into .NET 7 becomes harder with every day. Do you know what is missing to get this one in? Do we still need to reach consensus?

ViktorHofer · 2022-09-27T09:49:44Z

@smasher164 gentle ping regarding status. I just yesterday backported an APICompat change into release/7.0.1xx and that required Tactics approval (even for a non binary impacting change).

ViktorHofer · 2022-09-28T16:16:53Z

Great, this is now in. Can you please backport this change to release/7.0.1xx and fill in the servicing template, apply the servicing-consider label and send a mail to Tactics? Feel free to reach out to Eric or me offline regarding the template or the process.

…pe" (#27774) (#28233) * Revert "do not emit diagnostic when attribute argument is internal type (#27648)" This reverts commit c533dcc. * add test for internal type arguments

Revert "do not emit diagnostic when attribute argument is internal ty…

49fa7cb

…pe (#27648)" This reverts commit c533dcc.

smasher164 added the Area-ApiCompat label Sep 8, 2022

smasher164 requested a review from ViktorHofer September 8, 2022 21:04

smasher164 requested review from a team, ericstj and joperezr as code owners September 8, 2022 21:04

ericstj reviewed Sep 9, 2022

View reviewed changes

add test for internal type arguments

65c6944

smasher164 requested a review from ericstj September 12, 2022 21:02

ericstj reviewed Sep 13, 2022

View reviewed changes

ViktorHofer approved these changes Sep 28, 2022

View reviewed changes

ViktorHofer merged commit bab7883 into main Sep 28, 2022

ViktorHofer deleted the revert-27648-internal-type-attribute branch September 28, 2022 16:15

smasher164 mentioned this pull request Sep 28, 2022

[release/7.0.1xx] Revert "do not emit diagnostic when attribute argument is internal type (#27648)" #28233

Merged

ViktorHofer mentioned this pull request Mar 9, 2023

GenAPI outputs reference to internal type in assembly-level attribute #31098

Closed

	/// <summary>
	/// Determines if internal members should be validated.
	/// </summary>
	public readonly bool IncludeInternalSymbols;

Revert "do not emit diagnostic when attribute argument is internal type" #27774

Revert "do not emit diagnostic when attribute argument is internal type" #27774

Uh oh!

Conversation

smasher164 commented Sep 8, 2022

Uh oh!

ViktorHofer commented Sep 8, 2022

Uh oh!

smasher164 commented Sep 8, 2022

Uh oh!

ericstj commented Sep 9, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ericstj Sep 12, 2022 • 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!

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!

ViktorHofer commented Sep 21, 2022

Uh oh!

smasher164 commented Sep 21, 2022

Uh oh!

ViktorHofer commented Sep 22, 2022

Uh oh!

ViktorHofer commented Sep 27, 2022

Uh oh!

ViktorHofer commented Sep 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ericstj Sep 12, 2022 •

edited

Loading