search

JetBrains / resharper-fsharp

F# support in JetBrains Rider
Apache License 2.0
328 stars 53 forks source link

Over-zealous CompilerServices.Extension removal hint in 2023.3.1 #587

Closed bartelink closed 6 months ago

bartelink commented 7 months ago

My global.json refs a v6 SDK with rollforward as latestMajor (and I have V8 SDK installed)

image

While this hint is correct if you are using a V8 compiler (implemented in https://github.com/dotnet/fsharp/pull/13839), it's very wrong if your CI only supplies a V6 one (it'll error in F# or C# consuming code as the compiler does not add the implicitly required one at the type level)

Ideally, that hint should be subject to a guarantee of V7/V8 compiler being present

auduchinok commented 6 months ago

@bartelink Interesting, thank you for writing about it!

We've actually also had an issue in the detection of the language version recently (which is now fixed in the upcoming update), and we could suggest simplifying it this way when F# 8 compiler wasn't available locally either.

The problematic part with your case is we always use the current compiler/toolset info to determine the available version, and not what is allowed by the global.json besides the local ones. I'll talk to the team about it, thanks!

auduchinok commented 6 months ago

@bartelink Is there a reason for you to have that much difference between your local environment and CI?

While I agree that using global.json works great for allowing wider range of SDKs to be used by other contributors, there may still be various small and unexpected issues (breaking changes in the compilers, potentially incompatible compiler plugins (i.e. F# type providers, Roslyn analyzers) that could break build for someone else on another SDK. Also, the analyzers used during the local build (Roslyn ones for C#, or the upcoming F# ones planned for F# 9+) currently know nothing about the global.json, so they may suggest making changes that wouldn't be compatible with the language version that could be implied by global.json, just like we do in our analysis.

Currently we've decided that we would like to stick with checking the local environment only, because maintaining a table of SDKs, compilers, and their supported language versions and breaking changes would require quite some effort, and it seems that specifying the target version via LangVersion project property would solve this good enough for now. We might support checking if there's a gap between language versions in the global.json and in the local compiler in future, if we see more people have this problem and report issues similar to this one.

bartelink commented 6 months ago

That makes sense; I can put a LangVersion in Directory.build.props to match the implication of the version in global.json - it had just not occurred to me.

(reason for discrepancy is that prod images have V6 which is LTM, but brew install has 8 and I can't be bothered to figure out how to dual install and have everything everywhere see them all. TBH I have a local diff I keep stashing/unstashing to flip between latestRelease and latestMajor, which is a royal pain!)