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

Some mixed missed Equals nullable annotations #52166

Merged
merged 5 commits into from
Jun 2, 2021

Conversation

hrrrrustic
Copy link
Contributor

Decided to push it without splitting by areas because there are not so much changes

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@hrrrrustic
Copy link
Contributor Author

hrrrrustic commented May 1, 2021

public override bool Equals([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] object? obj) { throw null; }

The type or namespace name 'NotNullWhenAttributeAttribute' does not exist in the namespace 'System.Diagnostics.CodeAnalysis' (are you missing an assembly reference?)

Why "Attribute" was repeated twice?

@jeffhandley
Copy link
Member

public override bool Equals([System.Diagnostics.CodeAnalysis.NotNullWhenAttribute(true)] object? obj) { throw null; }

The type or namespace name 'NotNullWhenAttributeAttribute' does not exist in the namespace 'System.Diagnostics.CodeAnalysis' (are you missing an assembly reference?)

Why "Attribute" was repeated twice?

Weird -- I don't understand what's happening here either. @krwq can you review this and chime in if you've seen this before?

@stephentoub
Copy link
Member

Weird -- I don't understand what's happening here either.

The compiler is searching for the type, both as written and with an Attribute suffix added on (even though it already has one). It means the attribute isn't imported, which generally means either a) a using is missing or b) it's coming from some build targeting a framework that lacks the attribute and where we're not yet injecting an internal version.

@jeffhandley
Copy link
Member

Thanks, @stephentoub!

@hrrrrustic -- let us know if you need any help narrowing the build errors down from here.

@jeffhandley
Copy link
Member

Once this is merged, we'll need to edit the description on dotnet/docs#21202 to include the affected APIs from this PR.

@hrrrrustic
Copy link
Contributor Author

hrrrrustic commented May 27, 2021

a) a using is missing

What kind of using you're talking about? The error was in the ref file, I've never saw usings in it 😄
I tried to add

</ItemGroup>
    <ItemGroup>
    <ProjectReference Include="..\..\System.Runtime\ref\System.Runtime.csproj" />
</ItemGroup>

to Microsoft.Extensions.Logging.Abstractions.csproj (ref directory), but got
error NU1201: Project System.Runtime is not compatible with netstandard2.0 (.NETStandard,Version=v2.0). Project System.Runtime supports: net6.0 (.NETCoreApp,Version=v6.0)
and
error NU1201: Project System.Runtime is not compatible with net461 (.NETFramework,Version=v4.6.1). Project System.Runtime supports: net6.0 (.NETCoreApp,Version=v6.0)

So I assume it's not possible to use this attribute here yet (correct me if I'm wrong) and simply revert changes

@buyaa-n
Copy link
Contributor

buyaa-n commented May 27, 2021

@hrrrrustic NullableAttributes.cs is not included in .Net Framework, probably in netstandard too, seems you need to add the file like this

<Compile Include="$(RepoRoot)src\libraries\System.Private.CoreLib/src/System/Diagnostics/CodeAnalysis/NullableAttributes.cs" />

@hrrrrustic
Copy link
Contributor Author

Rebased on top to be above #53199

…crosoft.Extensions.Logging.Abstractions.csproj

Co-authored-by: Buyaa Namnan <buyankhishig.namnan@microsoft.com>
Copy link
Contributor

@buyaa-n buyaa-n 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!

@buyaa-n
Copy link
Contributor

buyaa-n commented Jun 1, 2021

The Wasm test failures seem fixed , but rerunning that CI leg is not fixing the failure, so closing the PR and reopening it back

Wasm.Build.Tests.NativeBuildTests.Relinking_ErrorWhenMissingEMSDK [FAIL]
      System.TypeInitializationException : The type initializer for 'Wasm.Build.Tests.BuildTestBase' threw an exception.
      ---- System.TypeLoadException : Could not load type 'System.IO.DirectoryInfo' from assembly 'System.Runtime, Version=6.0.0.0, Culture=neutral, PublicKeyToken=b03f5f7f11d50a3a'.
      Stack Trace:
        /_/src/tests/BuildWasmApps/Wasm.Build.Tests/BuildTestBase.cs(123,0): at Wasm.Build.Tests.BuildTestBase.ConfigWithAOTData(Boolean aot)
        /_/src/tests/BuildWasmApps/Wasm.Build.Tests/InvariantGlobalizationTests.cs(21,0): at Wasm.Build.Tests.InvariantGlobalizationTests.InvariantGlobalizationTestData(Boolean aot, RunHost host)

@buyaa-n buyaa-n closed this Jun 1, 2021
@buyaa-n buyaa-n reopened this Jun 1, 2021
@buyaa-n buyaa-n closed this Jun 1, 2021
@buyaa-n buyaa-n reopened this Jun 1, 2021
@buyaa-n buyaa-n merged commit be41ea2 into dotnet:main Jun 2, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 2, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants