Update nullability convention to new nullability metadata

[roji]

See https://github.com/dotnet/roslyn/blob/master/docs/features/nullable-metadata.md.

Makes #16370 pass.

[ajcvickers]

@roji I was hoping to have a PR against the preview 7 branch. But maybe this is better in some way?

[roji]

I was hoping to have a PR against the preview 7 branch

Unless I'm mistaken, the problem is that branch doesn't have #16370 yet (because it's failing), meaning that this PR would fail as well - it's a catch-22 of PRs...

If this PR passes we can merge both into preview7 - or if you prefer me to do it some other way let me know.

[roji]

And now it seems that conventions have to be thread-safe - updating for that.

[ajcvickers]

@roji Good point.
@dougbu Since this is a breaking change from Rosyln, we cannot be in a state that will pass both before and after the dependency update. This also needs to go into preview 7. What sequence of PRs would you like us to take?

[roji]

Note: it seems like there will be another minor metadata change - instead of byte[] there will be a ReadOnlySpan: dotnet/roslyn#36604.

We can take care of that when that comes.

[roji]

Note: pushed update to make the convention class thread-safe as per @AndriySvyryd's advice.

[ajcvickers]

Just a meta-point here. Let's not rush this. We need to get it fixed for preview 7 and to unblock the build, but there is time for that to happen. Let's make sure we get a good, high-quality fix in.

[AndriySvyryd]

My personal preference is to take out this convention from the default convention set builder for now.

[smitpatel]

PR #16370 cannot go to preview7 since it has preview8 packages from Extensions. Likely this breaking change in nullability is supposed to be post-preview7.

[roji]

My personal preference is to take out this convention from the default convention set builder for now.

You mean disabling nullability support by default? Is that because of the concurrency/caching that's going on, or some other issue? If the caching really seems problematic (would be good to know why), I can also remove it entirely, although that would potentially mean lots of reflection for every member.

PR #16370 cannot go to preview7 since it has preview8 packages from Extensions. Likely this breaking change in nullability is supposed to be post-preview7.

@smitpatel where are you seeing preview8 packages? I'm only seeing preview7 in eng/Versions.props...

[roji]

Seems like the commits are piling up in #16370, only 6ced844 (introduced an hour ago) references preview8. I'm suspecting this is not intended, and that we do need to take the first two commits - @dougbu can hopefully clarify.

[smitpatel]

Concurrency issues....
Either next time Extensions build new package for preview7, we will get PR against preview7 created automatically. Else we have to pick first 2 commit from set manually.

[dougbu]

My apologies if the responses below are out of order.

PR #16370 cannot go to preview7 since it has preview8 packages from Extensions. Likely this breaking change in nullability is supposed to be post-preview7.

…

This also needs to go into preview 7

We need to confirm this. If the newer Roslyn will be included in the preview 7 SDK, we need to react in our preview 7 branches.

Side question: Is the Roslyn change coming in with the new Arcade?

there will be another minor metadata change - … dotnet/roslyn#36604

I can't tell looking at that issue what milestone it's going into ("6.3" doesn't mean much to me). Do you have more information?

Seems like the commits are piling up in #16370, only 6ced844 (introduced an hour ago) references preview8. I'm suspecting this is not intended, and that we do need to take the first two commits - @dougbu can hopefully clarify.

It's not intended that #16370 has been delayed. But, all of it should be merged into 'master' once we have the correct reactions in place.

What sequence of PRs would you like us to take?

1. Figure out whether we need to change preview 7
2. If yes, let me know and I'll trigger whichever of the subscriptions we need
   • Prioritize this over anything related to our 'master' branch
3. Instead of creating a separate PR, update the bot's branch e.g. 'darc-master-769fb36e-2de6-4540-871b-a9f7104a785b' (for [master] Update dependencies from dotnet/arcade aspnet/Extensions #16370) if we only need this reaction in 'master'
4. Since both fixes require new dependencies, fix 'master' separately
   • Merge from `3.0-preview7' will be a no-op and that's fine

[roji]

@AndriySvyryd pushed commit which makes the convention store state in an annotation, can you please re-review to make sure I've done it right?

[roji]

@dougbu

We need to confirm this. If the newer Roslyn will be included in the preview 7 SDK, we need to react in our preview 7 branches.

@ajcvickers @divega who can we ping to confirm this? It seems like the first two commits need to go into preview7, and those are the ones that introduced this break - but it would be good to make sure.

Side question: Is the Roslyn change coming in with the new Arcade?

I'm not sure - I do know that the compiler is brought in via the <UsingToolMicrosoftNetCompilers> property that was recently added by @natemcmaster. But I'm not sure what exactly triggered the new version being brought in.

there will be another minor metadata change - … dotnet/roslyn#36604

I can't tell looking at that issue what milestone it's going into ("6.3" doesn't mean much to me). Do you have more information?

16.3 is probably the Visual Studio version this is supposed to go out in - but I don't know more. In any case it seems safe to say this won't be before preview8, and the change is expected to be quite trivial.

[dougbu]

We need to confirm this. If the newer Roslyn will be included in the preview 7 SDK, we need to react in our preview 7 branches.

@livarcocc can you help us understand what version of Roslyn will ship in preview 7?

I do know that the compiler is brought in via the <UsingToolMicrosoftNetCompilers> property

Yeah, I thought it was something like that. The property is implemented in the Arcade SDK and we use it in AspNetCore, AspNetCore-Tooling and Extensions as well as this repo. Arcade presently brings in the 3.3.0-beta1-19327-03 version of the Microsoft.Net.Compilers.Toolset package but I don't know what version is destined for preview 7's core-sdk.

[dougbu]

The version of Roslyn that will ship in preview 7 is found in https://github.com/dotnet/toolset/blob/a75492eb4d209ba79e4a8b8c154f1035f80504df/eng/Version.Details.xml#L24-L27:

    <Dependency Name="Microsoft.NETCore.Compilers" Version="3.2.0-beta4-19326-12">
      <Uri>https://github.com/dotnet/roslyn</Uri>
      <Sha>72444d34039295fa7b6d9580c4be528b4275e79c</Sha>
    </Dependency>

That version looks older than what Arcade is giving us (3.3.0-beta1-19327-03) but I can't tell whether dotnet/roslyn@72444d3 includes these nullability changes -- commit might be related but it's hard to be sure.

[divega]

@agocke and @cston, could you please help us figure out whether we need to react to changes in nullability attributes in preview 7? In what state are things in Roslyn for preview7?

[dougbu]

Everyone, I think I have an easy way to test exactly what changes we need as well as provide a base for any Roslyn reactions. I'll open a draft PR to depend directly on the Roslyn package (taking the Arcade SDK out of the middle) in this repo's `release/3.0-preview7' branch. Will get that started after dinner…

[roji]

Updated and squashed, everything should be OK now.

Will wait for feedback on #16394 to know whether this needs to go into preview7, master, etc.

[dougbu]

Because the dependency versions don't flow from our preview branches back into 'master', need to do 'master' separately anyhow. Please merge this into the darc branch ASAP (modulo any comments that still need to be addressed) to unblock #16370.

[roji]

Merged into the darc branch - let me know about next steps for preview7.

[ajcvickers]

@roji In tactics it was confirmed that we are currently using the wrong SDK for the preview 7 branch. This is being worked on. /cc @dougbu

From our side, please prepare a PR for the preview 7 branch so we can merge it as soon as the SDK has flowed here.

[roji]

PR is ready: #16403