-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
Comments
Quick test after changing to 10 NRT warnings in |
I would be fine with taking a PR which contains nullability fixes (for example, adding |
Since roslyn haven't fully opt-in NRT, there may be further bugs after one-time fix. How about adding |
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 |
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 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:
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 |
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
|
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
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 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. |
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 |
Interesting thoughts: |
OK I do find NRT bug in public API. Changing should be generally OK since they are binary compatible, but how about versioning? |
@jaredpar Blocker find:
Do I need to create a separated issue? |
@jasonmalinowski who is the right team to route the ImageCatalog issue to? |
@jaredpar What's the context here? 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 immediate task @huoyaoyuan is looking at is better There isn't a huge hurry in doing this but it's a transition we usually start making around this time. |
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. 😄 |
Yes, EditorFeatures don't run on net5.0. But there are more dependencies blocked. Workspaces unit tests actually dependent on EditorFeatures. |
Should we though? Basically I see this as taking the slice of Roslyn which targets .NET core today and moving it from Anything which can't be referenced after moving to |
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. |
I'm not following. If it's |
@jaredpar: what's the question? |
My previous commont where I asked
|
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. |
I've met a NRT hole in #50820
The cause is here:
roslyn/src/Workspaces/SharedUtilitiesAndExtensions/Compiler/CSharp/Extensions/SyntaxTreeExtensions.cs
Lines 86 to 90 in 70b74e3
NRT analysis fails to catch this because LINQ is not annotated in netcoreapp3.1.
The text was updated successfully, but these errors were encountered: