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

NullabilityInfoContext returns incorrect result when code analysis attributes are used in a disabled context #63848

Closed
madelson opened this issue Jan 16, 2022 · 2 comments · Fixed by #64143

Comments

@madelson
Copy link
Contributor

Description

Attributes like DisallowNull and MaybeNull are still respected by the compiler even when the code they are annotating is otherwise nullable-oblivious. NullabilityInfoContext should do the same.

Reproduction Steps

public void Main()
{
        var context = new NullabilityInfoContext();
        
        var disallowNullInfo = context.Create(typeof(DisabledWithCodeAnalysisAttributes).GetProperty("DisallowNull")!);
        Console.WriteLine(disallowNullInfo.ReadState); // Unknown
        Console.WriteLine(disallowNullInfo.WriteState); // Unknown (should be NotNull)

        var maybeNullInfo = context.Create(typeof(DisabledWithCodeAnalysisAttributes).GetProperty("MaybeNull")!);
        Console.WriteLine(maybeNullInfo.ReadState); // Unknown (should be Nullable)
        Console.WriteLine(maybeNullInfo.WriteState); // Unknown
}

#nullable disable
    private class DisabledWithCodeAnalysisAttributes
    {
        [DisallowNull] public string DisallowNull { get; set; }
        [MaybeNull] public string MaybeNull { get; set; }
    }
#nullable enable

Expected behavior

The property DisallowNull should have a NotNull write state because the following produces a compiler warning (CS8625):

new DisabledWithCodeAnalysisAttributes().DisallowNull = null;

The property MaybeNull should have a Nullable read state because the following produces a compiler warning (CS8600):

string s = new DisabledWithCodeAnalysisAttributes().MaybeNull;

Actual behavior

All states are Unknown.

Regression?

No response

Known Workarounds

No response

Configuration

VS 17.0.2, .NET 6, Windows 10 x64.

I don't have any reason to believe this is configuration specific.

Other information

CC @Shane32

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Jan 16, 2022
@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this issue. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Jan 16, 2022

Tagging subscribers to this area: @dotnet/area-system-reflection
See info in area-owners.md if you want to be subscribed.

Issue Details

Description

Attributes like DisallowNull and MaybeNull are still respected by the compiler even when the code they are annotating is otherwise nullable-oblivious. NullabilityInfoContext should do the same.

Reproduction Steps

public void Main()
{
        var context = new NullabilityInfoContext();
        
        var disallowNullInfo = context.Create(typeof(DisabledWithCodeAnalysisAttributes).GetProperty("DisallowNull")!);
        Console.WriteLine(disallowNullInfo.ReadState); // Unknown
        Console.WriteLine(disallowNullInfo.WriteState); // Unknown (should be NotNull)

        var maybeNullInfo = context.Create(typeof(DisabledWithCodeAnalysisAttributes).GetProperty("MaybeNull")!);
        Console.WriteLine(maybeNullInfo.ReadState); // Unknown (should be Nullable)
        Console.WriteLine(maybeNullInfo.WriteState); // Unknown
}

#nullable disable
    private class DisabledWithCodeAnalysisAttributes
    {
        [DisallowNull] public string DisallowNull { get; set; }
        [MaybeNull] public string MaybeNull { get; set; }
    }
#nullable enable

Expected behavior

The property DisallowNull should have a NotNull write state because the following produces a compiler warning (CS8625):

new DisabledWithCodeAnalysisAttributes().DisallowNull = null;

The property MaybeNull should have a Nullable read state because the following produces a compiler warning (CS8600):

string s = new DisabledWithCodeAnalysisAttributes().MaybeNull;

Actual behavior

All states are Unknown.

Regression?

No response

Known Workarounds

No response

Configuration

VS 17.0.2, .NET 6, Windows 10 x64.

I don't have any reason to believe this is configuration specific.

Other information

CC @Shane32

Author: madelson
Assignees: -
Labels:

area-System.Reflection, untriaged

Milestone: -

@buyaa-n buyaa-n removed the untriaged New issue has not been triaged by the area owner label Jan 19, 2022
@buyaa-n buyaa-n added this to the 7.0.0 milestone Jan 19, 2022
madelson added a commit to madelson/runtime that referenced this issue Jan 22, 2022
In several cases, NullabilityInfoContext would return values which were
out of sync with the C# compiler's interpretation of the nullability
metadata. This fixes the following cases:
* The `NullablePublicOnly` attribute was not considered when analyzing
private constructor parameters.
* CodeAnalysis attributes on indexer properties were not recognized.
* CodeAnalysis attributes were not recognized in a `#nullable disabled`
context.
* Private value-typed members lacking nullable annotations were not
properly marked as nullable/notnull.
* Mixing CodeAnalysis attributes with opposite meanings (e. g.
`AllowNull` and `DisallowNull` produced the wrong result.
* Analysis of members inherited from generic base types did not
incorporate the nullable metadata associated with the inheritance.

Fix dotnet#63555
Fix dotnet#63846
Fix dotnet#63847
Fix dotnet#63848
Fix dotnet#63849
Fix dotnet#63853
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 22, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 26, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Feb 26, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants