Skip to content
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.

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

Code changes to allow 'using static unsafe' #67344

Merged
merged 23 commits into from Mar 21, 2023

Conversation

CyrusNajmabadi
Copy link
Member

@CyrusNajmabadi CyrusNajmabadi commented Mar 16, 2023

Fixes #67329

Relates to test plan #56323

@CyrusNajmabadi CyrusNajmabadi marked this pull request as ready for review March 17, 2023 20:07
@CyrusNajmabadi CyrusNajmabadi requested a review from a team as a code owner March 17, 2023 20:07
@@ -904,37 +904,37 @@ public void UsingStaticUnsafeNonAlias()
}

[Fact]
public void AliasUsingDirectivePredefinedType_CSharp11()
public void UsingStaticUnsafe_SafeType_CSharp11_NoUnsafeFlag()
Copy link
Member Author

Choose a reason for hiding this comment

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

bad github diffs. tests were added, pushing prior tests down. github registering as a change.

@@ -863,46 +863,6 @@ public void UsingUnsafeNonAlias()
EOF();
}

[Fact]
Copy link
Member Author

Choose a reason for hiding this comment

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

this test moved down so it could remain with the rest of the variants added for it. I couldn't add teh variants next to it, or github blewup thinking all the tests had changed.

@CyrusNajmabadi
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jcouv jcouv self-assigned this Mar 20, 2023
// Make sure 'unsafe' is even allowed in this language version, reporting an error
// otherwise. If it is available, ensure that the user passed the '/unsafe' flag to
// to the compiler.
if (MessageID.IDS_FeatureUsingTypeAlias.CheckFeatureAvailability(diagnostics, usingDirective, usingDirective.UnsafeKeyword.GetLocation()))
Copy link
Member

@jcouv jcouv Mar 20, 2023

Choose a reason for hiding this comment

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

Same comment here. I would not make CheckUnsafeModifier conditional on langver check. Comment above will need to be updated accordingly #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.

done.

Copy link
Member

Choose a reason for hiding this comment

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

Was a test affected?

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. many tests were affected by this.

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 16)

@CyrusNajmabadi
Copy link
Member Author

@jcouv ptal :)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

Done with review pass (iteration 20)

Copy link
Member

@jcouv jcouv left a comment

Choose a reason for hiding this comment

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

LGTM Thanks (iteration 20)

}

[Fact]
public void UsingStatic_UnsafeType_CSharp11_NoUnsafeFlag()
Copy link
Member

@cston cston Mar 20, 2023

Choose a reason for hiding this comment

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

UsingStatic_UnsafeType_CSharp11_NoUnsafeFlag

It looks like the added tests each have 4 variants, { C# 11, C# next } x { no -unsafe, -unsafe }, and each test tests binding and parsing separately. But the results of the parser should be the same across all variants (and the parser does not depend on CSharpCompilationOptions which contains the unsafe setting).

Given that, would it make sense to combine the variants? For instance:

[Theory]
[InlineData(LanguageVersion.CSharp10)]
[InlineData(LanguageVersion.CSharp11)]
public void UsingStatic_UnsafeType(LanguageVersion languageVersion)
{
    var parseOptions = TestOptions.Regular.WithLanguageVersion(languageVersion);
    var text = "...";

    if (languageVersion == LanguageVersion.CSharp10)
    {
        CreateCompilation(text, parseOptions: parseOptions).VerifyDiagnostics(...);
        CreateCompilation(text, parseOptions: parseOptions, options: TestOptions.UnsafeDebugDll).
            VerifyDiagnostics(...);
    }
    else
    {
        CreateCompilation(text, parseOptions: parseOptions).VerifyDiagnostics(...);
        CreateCompilation(text, parseOptions: parseOptions, options: TestOptions.UnsafeDebugDll).
            VerifyDiagnostics(...);
    }

    UsingTree(text, options: parseOptions);
    N(SyntaxKind.CompilationUnit);
    {
        ...
    }
}

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 find theories, which then have different behavior on units, nigh unreadable. I spend more time just trying to understand what the final test is actually testing.

It's also a huge pita when one variant of the test fails, and you continually have to revise and run all the other variants.

I'm a big fan of a test having an input and validating it. A true unit test.

@CyrusNajmabadi CyrusNajmabadi enabled auto-merge (squash) March 21, 2023 00:14
auto-merge was automatically disabled March 21, 2023 00:14

Pull request was closed

@CyrusNajmabadi CyrusNajmabadi merged commit 35391a8 into dotnet:main Mar 21, 2023
26 checks passed
@CyrusNajmabadi CyrusNajmabadi deleted the usingStatic branch March 21, 2023 00:14
@ghost ghost added this to the Next milestone Mar 21, 2023
@allisonchou allisonchou modified the milestones: Next, 17.6 P3 Mar 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Need to add support for using unsafe static <Type>;
4 participants