Skip to content

Conversation

@KevinRansom
Copy link
Contributor

This enables FSharp4.7 language features by default in the compiler. And updates the tests to no longer use: "--langversion:preview"

Will need updating after: #7195 is merged.

Copy link
Member

Choose a reason for hiding this comment

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

Please don't change these numbers in future.

And I still believe it'd be better to have separate enums for language versions and features.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@auduchinok.
They aren't public and shouldn't be relied on. In fact, I wouldn't mind deleting the individual feature ID's to remove the possibility that we try to add a --enablefeature:WildCardInForLoop option.

Copy link
Member

Choose a reason for hiding this comment

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

It's probably reasonable to think they may become public in FCS at some point when tooling wants to see which features are available.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@auduchinok,

The smallest subdivision of the language is language version, from it one can infer the set of features available. If we choose to add or I suppose remove features, then we will release a new language version, similarly to how we release new nuget packages and assembly versions for FSharp.Core.

It is not our intention to make the information in these structures public. I can see a good argument for removing

 | Preview = 0
 | LanguageVersion46 = 1
 | LanguageVersion47 = 2

from the LanguageFeature enumeration, given that it has so far not proven to be necessary to do a SupportsFeature.LanguageVersion4.7 call.

Anyway, my 2 cents, and probably not even worth that amount of coin.

Copy link
Member

Choose a reason for hiding this comment

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

The smallest subdivision of the language is language version, from it one can infer the set of features available.

@KevinRansom Thanks for your reply. I don't, however, realise how it should be inferred by external tools using FCS given the whole module is currently internal (unless the whole logic is reimplemented in each tool). There may be situations where tooling behaviour can also change depending on whether some feature is supported in particular language version.

Copy link
Contributor

Choose a reason for hiding this comment

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

@auduchinok Which situations? The way it works in Roslyn, which is the model we're adopting, is that tools assume latest. But because diagnostics will kick in if something doesn't actually compile, the only weird thing is displaying information when you're in an invalid state.

Here's an example in C# 7.3 but using nullable:

image

I could believe two opposing arguments:

  1. That it is incorrect behavior to show string?, since it's not allowed in the first place.
  2. That it is correct to show string?, since it is what the programmer wrote

Since this can be resolved either by turning string? to string or setting LangVersion ot 8.0 (or preview), it's unclear to me which is the correct behavior. But it's certainly easier from a tooling standpoint to not flow LangVersion into every tooling feature.

Copy link
Member

Choose a reason for hiding this comment

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

@cartermp I don't have a particular example in mind but I can definitely imagine situations where an IDE could, for instance, produce different generated code or do code formatting differently depending on the language version in future.

@KevinRansom KevinRansom force-pushed the enable47FeaturesByDefault branch from 486123a to c60d08c Compare July 12, 2019 00:42
@KevinRansom KevinRansom merged commit f5f7f0f into dotnet:release/fsharp47 Jul 12, 2019
@KevinRansom KevinRansom deleted the enable47FeaturesByDefault branch August 2, 2019 19:13
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.

4 participants