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

Target compiler with net5.0 to cover NRT issues with LINQ #50822

Closed
huoyaoyuan opened this issue Jan 27, 2021 · 25 comments
Closed

Target compiler with net5.0 to cover NRT issues with LINQ #50822

huoyaoyuan opened this issue Jan 27, 2021 · 25 comments
Assignees
Labels
Area-Compilers Bug fabric-bot-test Testing the impact of changes to the fabric bot Need More Info The issue needs more information to proceed.
Milestone

Comments

@huoyaoyuan
Copy link
Member

I've met a NRT hole in #50820

The cause is here:

public static TypeDeclarationSyntax GetContainingTypeDeclaration(
this SyntaxTree syntaxTree, int position, CancellationToken cancellationToken)
{
return syntaxTree.GetContainingTypeDeclarations(position, cancellationToken).FirstOrDefault();
}

NRT analysis fails to catch this because LINQ is not annotated in netcoreapp3.1.

@dotnet-issue-labeler dotnet-issue-labeler bot added Area-Compilers untriaged Issues and PRs which have not yet been triaged by a lead labels Jan 27, 2021
@huoyaoyuan
Copy link
Member Author

Quick test after changing to net5.0:

10 NRT warnings in Microsoft.CodeAnalysis. No critical(real bug) one.
Ambiguous name for IReadOnlySet and ReferenceEqualityComparer.

@RikkiGibson
Copy link
Contributor

I would be fine with taking a PR which contains nullability fixes (for example, adding ? annotations) found by changing the target framework locally, but probably not a PR that actually changes the target framework. I think we try to target the most recent LTS runtime as a matter of practice. @jaredpar

@huoyaoyuan
Copy link
Member Author

Since roslyn haven't fully opt-in NRT, there may be further bugs after one-time fix.

How about adding net5.0 (not replacing netcoreapp3.1)?

@RikkiGibson
Copy link
Contributor

We're reluctant to add more target frameworks due to the impact on build time, artifact size, IDE experience, etc.

I don't think I have the full context on why we have kept the .NET core target at netcoreapp3.1, though, so I don't know yet when we will be able to change it to get a more persistent benefit.

@jaredpar
Copy link
Member

The underlying motivation here is how the SDK ships. At any given time we are usually shipping two different SDKs: current RTM and next RTM. Back in say November this was netcoreapp3.1 and net5.0 respectively. Both of the SDKs are evolving and shipping at the same time and teams that push tools into those SDKs need to account for this.

The compiler, and pretty much every team that ships tools in the SDK, account for this by just shipping one version of the tool in both SDKs. That means that a .NET 3.1 SDK and .NET 5 SDK that are released at the same time will typically have the exact same C# compiler included in it (yet another reason compat is really important). Given that we must necessarily target the TFM of the oldest SDK that we are shipping inside of.

To change the minimum target framework used in our csc / vbc builds we essentially have to have assurances from the SDK team that they are no longer actively shipping an SDK that uses that runtime. This is usually a simple email in the form of:

Just making sure we're not producing new 3.1.X builds anymore and we're safe to bet on net5.0 as the tools runtime in the SDK.

Given that .NET 5 SDK is shipped now and we're looking towards the .NET 6 previews we are hitting that point where the switch can be made. Generally though we don't do this until the beginning of a VS preview cycle. Now that master is targeting 16.10 it would be an appropriate time to do it here. At the same time there is generally not a huge rush to do this. In the past we probably lagged ~3 months before we made the switch because we didn't have strong motivations to do it.

This round though we have another item to consider: library compat. The 3.1 SDK cycle is the first time we shipped netcoreapp3.1, before that it was netstandard2.0 everywhere. The decision now is whether we move that to net5.0 or add net5.0 as an additional target. The general flow of the SDK is to just move not to keep adding targets (it's simply not sustainable over the long haul given how often we release). But it is something I wanted to be explicit about.

@huoyaoyuan
Copy link
Member Author

Thanks for explanations. I have some additional questions:

3.1 is LTS release and 3.1.4xx SDK is still shipping. What is the compiler shipped in it using? (Also for 2.1.812)

There are some name collisions when trying to move to net5.0. This is caused by the runtime shipping new API with same or similar functionality. (namely IReadOnlySet and ReferenceEqualityComparer. Since roslyn must keep netstandard2.0 target, and the runtime does not provide an OOB package for the API, we cannot move to them. What's the selected approach here?

  • Rename ours to avoid name collisions
  • Keep ours and use quailification
  • Use #if to let ours simulate runtime ones when not available

@jaredpar
Copy link
Member

3.1 is LTS release and 3.1.4xx SDK is still shipping.

There is a difference between shipping 3.1.Xxx and 3.1.xXX. The former is a point release and that can have new behavior, features, etc ... Pretty much every time you get a new point release of VS, say 16.7, then you get a new accompanying SDK release, say 3.1.400.

The latter though are servicing releases. These are targeted fixes that come from specific servicing branches. The changes on master aren't flowing to 3.1.4xx. Instead only changes from release/dev16.7 are flowing there. Hence changing master doesn't impact that flow.

There are some name collisions when trying to move to net5.0. ... What's the selected approach here?

Usually we take these on a case by case basis. There are factors like "is the type we ship public?" that can change the outcome. Generally though we qualify the names and move on. In some very rare cases we need an #if to deal with it.

Note: we still need to chat with the SDK before we make the change and most of us are fairly heads down on 16.9 bugs at the moment.

@huoyaoyuan
Copy link
Member Author

Note: we still need to chat with the SDK before we make the change and most of us are fairly heads down on 16.9 bugs at the moment.

I can start a prototype branch. Maybe 17.0 is also an acceptable point because it's a major version.

@jaredpar
Copy link
Member

Honestly I think 16.10 is fine and generally I don't even need a branch for it. A PR is usually enough to make the change. It's just a lot of manual labor to find replace all the netcoreapp3.1 with net5.0. There are always a few random exceptions that you have to work through.

@huoyaoyuan
Copy link
Member Author

Interesting thoughts:
Since IReadOnlySet<T> is generic, it cannot be qualified by using alias. Since there are references to other type in the namespace and using Roslyn.Utilities are still kept, the fully qualification can be easily broken by IDE if netstandard2.0 target is selected.

@huoyaoyuan
Copy link
Member Author

OK I do find NRT bug in public API. Changing should be generally OK since they are binary compatible, but how about versioning?

@huoyaoyuan
Copy link
Member Author

@jaredpar Blocker find:

Microsoft.VisualStudio.ImageCatalog adds a superfluous framework reference to WPF on netcoreapp3.1, which prevents EditorFeatures (non-WPF) to move. .NET 5 SDK disallows WPF reference on neutral platform and only allows on Windows.
The implementation of the package doesn't require WPF or Windows at all. Please contact them to remove the reference. The issue is present on latest package 16.9.30927.236.

Do I need to create a separated issue?

@jaredpar
Copy link
Member

@jasonmalinowski who is the right team to route the ImageCatalog issue to?

@jasonmalinowski
Copy link
Member

@jaredpar What's the context here? Is this targeting projects against net5.0 for better validation of nulls in Roslyn, or something else?

@huoyaoyuan
Copy link
Member Author

Is this targeting projects against net5.0 for better validation of nulls in Roslyn, or something else?

@jasonmalinowski Yes. I've opened PR for issues in Compiler, Workspace and Features layer, cherry-picked without framework change.
The superfluous reference causes build error in net5.0, and blocks EditorFeatures layer and beyond.

@jaredpar
Copy link
Member

jaredpar commented Feb 1, 2021

@jasonmalinowski

Is this targeting projects against net5.0 for better validation of nulls in Roslyn, or something else?

The immediate task @huoyaoyuan is looking at is better null validation. In the medium term though the compiler, as well as pretty much all the other tools in the SDK, will be moving to net5.0. Now that netcoreapp3.1 is done, net5.0 is the new RTM, we will begin the process of moving over to that.

There isn't a huge hurry in doing this but it's a transition we usually start making around this time.

@jasonmalinowski
Copy link
Member

I guess at some point though we hit layers that emphatically don't run on net5.0. This is getting close to (but not crossing) the border so just wanted to confirm we weren't trying to do something else. 😄

@huoyaoyuan
Copy link
Member Author

Yes, EditorFeatures don't run on net5.0. But there are more dependencies blocked. Workspaces unit tests actually dependent on EditorFeatures.

@jaredpar
Copy link
Member

jaredpar commented Feb 1, 2021

I guess at some point though we hit layers that emphatically don't run on net5.0.

Should we though? Basically I see this as taking the slice of Roslyn which targets .NET core today and moving it from netcoreapp3.1 to net5.0. That should, in theory, be a safe operation. Anything that slice is referencing is either netstandard2.0 or earlier or netcoreapp3.1 or earlier. All of which should be legal to reference from net5.0.

Anything which can't be referenced after moving to net5.0 is seemingly a bug that should be fixed. Roslyn may be the first one that hits the issue but likely won't be the last.

@jasonmalinowski
Copy link
Member

Workspaces unit tests actually dependent on EditorFeatures.

Yes, but that's the real problem that should be fixed. 😄 That said it's an awful tangled mess so I don't imagine it's remotely easy, but there's other problems not fixing that is going to cause.

@jaredpar: so everything above EditorFeatures is all net472 targeting. So the wall is going to be immediately after what @huoyaoyuan is touching. EditorFeatures project specifically is an odd one in that we only recently switched it from targeting net472 but there's a bunch of strange caveats there, like it uses ITextView where there's no actual implementation other than the WPF and Cocoa one...both of which are net472 at the moment AFAIK. Put another way: this is running into the dragons we knew were hiding and are currently in the process of evicting from our codebase (as in literally we have in-flight insertions with partners to fix as I write this), but we're not done yet.

@jaredpar
Copy link
Member

jaredpar commented Feb 2, 2021

so everything above EditorFeatures is all net472 targeting. So the wall is going to be immediately after what @huoyaoyuan is touching.

I'm not following. If it's net472 targeting then there is no expectation that it moves to net5.0 here, it stays where it is. The only process here is moving netcoreapp3.1 to net5.0.

@jaredpar jaredpar added Bug and removed untriaged Issues and PRs which have not yet been triaged by a lead labels Feb 2, 2021
@jaredpar jaredpar self-assigned this Feb 2, 2021
@jaredpar jaredpar added the Need More Info The issue needs more information to proceed. label Feb 2, 2021
@jaredpar jaredpar added this to the 16.10 milestone Feb 2, 2021
@jaredpar
Copy link
Member

jaredpar commented Mar 8, 2021

@jasonmalinowski ?

@jasonmalinowski
Copy link
Member

@jaredpar: what's the question?

@jaredpar
Copy link
Member

jaredpar commented Mar 9, 2021

@jasonmalinowski

My previous commont where I asked

I'm not following. If it's net472 targeting then there is no expectation that it moves to net5.0 here, it stays where it is. The only process here is moving netcoreapp3.1 to net5.0.

@jinujoseph jinujoseph modified the milestones: 16.10, 17.0 Jul 16, 2021
@jaredpar jaredpar modified the milestones: 17.0, 17.1 Aug 3, 2021
@ghost ghost added the fabric-bot-test Testing the impact of changes to the fabric bot label Aug 13, 2021
@ghost ghost closed this as completed Sep 15, 2021
@ghost
Copy link

ghost commented Sep 15, 2021

Closing this issue as we've seen no reply to the request for more information. If you are able to get the requested information, please add it to the issue and we will retriage it.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug fabric-bot-test Testing the impact of changes to the fabric bot Need More Info The issue needs more information to proceed.
Projects
None yet
Development

No branches or pull requests

5 participants