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

fix regression in editing .NET Core scripts in devenv.exe #13994

Merged
merged 5 commits into from
Oct 6, 2022

Conversation

dsyme
Copy link
Contributor

@dsyme dsyme commented Sep 27, 2022

Fixes #13993

The TFM we need to return from FxResolver is the correct TFM to use for script analysis (which is often the running TFM, but not always, in-editor). See notes in code

This resurrects a small portion of the code removed in #13993 (which removed lots of other ugly code, but this bit should not have been removed, and is the only technique we know of to get the target TFM based on an SDK dir)

@vzarytovskii I'll target this to dev17.4

@dsyme dsyme changed the base branch from main to release/dev17.4 September 27, 2022 14:48
@dsyme
Copy link
Contributor Author

dsyme commented Sep 27, 2022

Note that this repairs .NET Framework-executing tooling analyzing .NET Core scripts. It does not repair the other way around (.NET Core-executing tooling analyzing .NET Framework scripts)

I'd like to work out if that scenario matters

@baronfel My understanding is that Ionide runs as .NET Core and always analyses scripts as .NET Core.

@auduchinok What's the situation with Rider? Does it run as .NET Core or .NET Framework tooling? If .NET Core then do you support editing scripts with .NET Framework references?

@baronfel
Copy link
Member

@dsyme FSAC actually still surfaces the option for .NET Framework checking, but I have no data on usage rates, especially because the FSI that we spawn in-editor is only ever .NET SDK-shipped (and therefore .NET Runtime, not Framework). Probably safe to deprecate on our part.

@dsyme
Copy link
Contributor Author

dsyme commented Sep 27, 2022

@baronfel Looking at the previous code I think in-editor .NET Core tooling had always resolved #r "nuget: ..." incorrectly for .NET Framework scripts, using a .NET Core TFM for the resolution.

So it won't be a change in behaviour (though would not be correct if we ever want .NET Core tooling to be able to analyze .NET Framework scripts)

@dsyme
Copy link
Contributor Author

dsyme commented Sep 27, 2022

I've added tests to this

@auduchinok
Copy link
Member

auduchinok commented Sep 28, 2022

@auduchinok What's the situation with Rider? Does it run as .NET Core or .NET Framework tooling? If .NET Core then do you support editing scripts with .NET Framework references?

@dsyme Thanks for asking about it! We've switched to .NET Core on Windows some time ago.

I haven't done anything special to support the .NET Framework scripting yet, and there're some bug reports in the scripts area that I'm going to look into in 2022.3. They're mostly about choosing incorrect TFM and/or toolset which makes #r "nuget: ..." resolution fail. I should probably write you when I'm investigating it a bit later and have more details?

Copy link
Member

@KevinRansom KevinRansom left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry about that. This looks like a good fix.

@KevinRansom KevinRansom merged commit f07ccb1 into dotnet:release/dev17.4 Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Regression: Scripts are being edited as .NET Framework, not .NET Core
6 participants