Remove ML compatibility

[kerams]

Description

Implements https://github.com/fsharp/fslang-suggestions/issues/1407

• Released keywords asr, land, lor, lsl, lsr, lxor (mod continues to be reserved)
• Removed support for .ml and .mli source files
• Removed #light and #indent directives
• Removed --light, --indentation-syntax, --no-indendation-syntax, --ml-keywords and --mlcompatibility compiler/fsi flags
• Removed parsing support for deprecated ML (non-light) constructs, meaning there's no longer any warning or error
• Removed ML compatibility tests
  • The changes in other test files only include removing #light from source code. There are hundreds of these files, but they can be safely skipped during review

Checklist

• [x] Test cases added

• [x] Performance benchmarks added in case of performance changes

• [ ] Release notes entry updated:

  Please make sure to add an entry with short succinct description of the change as well as link to this pull request to the respective release notes file, if applicable.

  Release notes files:

  • If anything under src/Compiler has been changed, please make sure to make an entry in docs/release-notes/.FSharp.Compiler.Service/<version>.md, where <version> is usually "highest" one, e.g. 42.8.200
  • If language feature was added (i.e. LanguageFeatures.fsi was changed), please add it to docs/release-notes/.Language/preview.md
  • If a change to FSharp.Core was made, please make sure to edit docs/release-notes/.FSharp.Core/<version>.md where version is "highest" one, e.g. 8.0.200.

  Information about the release notes entries format can be found in the documentation. Example:

  • More inlines for Result module. (PR #16106)
  • Correctly handle assembly imports with public key token of 0 length. (Issue #16359, PR #16363) *while! (Language suggestion #1038, PR #14238)

  If you believe that release notes are not necessary for this PR, please add NO_RELEASE_NOTES label to the pull request.

[github-actions[bot]]

:heavy_exclamation_mark: Release notes required

---

:white_check_mark: Found changes and release notes in following paths:

Change path	Release notes path	Description
src/FSharp.Core	docs/release-notes/.FSharp.Core/10.0.200.md

[kerams]

What do you think about keeping the parser rules and just always producing the errors?

That's been the case since ML compatibility was deprecated. The only way to get rid of it was the combination of the 2 flags. We could keep the parser rules, but one of the benefits of doing this was a slight parser/lexer simplication, and I'm not sure what would be the practical benefit of preserving them.

[auduchinok]

We could keep the parser rules, but one of the benefits of doing this was a slight parser/lexer simplication, and I'm not sure what would be the practical benefit of preserving them.

I think keeping the simple rules that don't require any additional support (like the type arguments in parens) would be beneficial. I see it as a sort-of error recovery rules.

[T-Gro]

Yes, long term code simplification should be the motivator here, since the rules were there but obsolete for many years. Old SDKs/nugets will continue to work if anyone wishes to parse F# files which are out of support.

(and it is always possible to e.g. copy existing parser rules+code and build a dedicated "ML-style-syntax-tree nuget" with it.)

[kerams]

This is going slower than I had hoped. Azure devops doesn't list some tests results, so I have to sift through the logs. I also can't run a lot of tests on my machine (and therefore easily update baselines) because of FSharp.Core conflicts (among other issues).

[T-Gro]

because of FSharp.Core conflicts

Is it because of the changes this PR did? Right now I cannot see why the bootstrap process via build scripts should complain, but maybe I overlooked a change.

[kerams]

As ever, git clean -xdf fixes it

[kerams]

What the hell is this?

Did I accidentally nuke some important bit or is the local runner environment acting up?

[auduchinok]

What the hell is this?

@kerams It looks similar to #15908.

[T-Gro]

The first error is legit - the error message is build up by the parsing state machine by the tokens which are allowed next. i.e. it is right that it triggers for your change, as the parser now behaves differently. EDIT - I was too quick, Eugene is right. This might be Debug config behavior just as well.

The second indicates a failed optimization, are you running -c Release and release versions of all things?

[kerams]

Yeah, debug... Thanks to you both.

By the way, would it be possible to update the required SDK to 10.0.1 in main? I do have rc2 installed, but VS really doesn't like it so I have to edit global.json.

[T-Gro]

Yep, sure. Done at https://github.com/dotnet/fsharp/pull/19149.

[kerams]

This is pretty much ready.

What if this PR (first for the de-supporting activity) ALREADY did a hard error on lang version [4.6..7] and point people to https://dotnet.microsoft.com/en-us/download/dotnet/10.0 if they can't migrate yet

I could do that here, but there are a bunch of tests targeting lower than 8, so the PR would become even messier than it already is.

Regarding release notes, should they really go in the files suggested in the first comment?

[T-Gro]

Regarding release notes, should they really go in the files suggested in the first comment?

Yes, this is correct. Compiler service API has changed, library has changed and language has changed as well.

I could do that here, but there are a bunch of tests targeting lower than 8, so the PR would become even messier than it already is.

I am open for ideas on how to enable it for tests temporarily and postpone cleanup - a lot of tests can be plain deleted because they will be no longer testing a relevant setup. However, some were using langVersion for convenience, but still test relevant things - I see how this could end up increasing the diff a lot.

We should have a user-friendly warning and optimize for the F# code maintainer not aware of what is happening here or over at fslang-suggestions. From their perspective, a new update to VS can just "randomly break unrelated code files" and we can prevent a lot of confusion if there is a direct message saying "langVersion xyz is out of support in F#X, please download Y".

A --test flag or an environment variable might be the mechanism to temporarily allow the legacy lang versions for tests, and start warning for them in the product.

[kerams]

Added. I just throw on <8, there's no SDK <-> language version matrix. An exercise for someone else down the line;)

"Language version '%s' is out of support. The last .NET SDK supporting it is available at https://dotnet.microsoft.com/en-us/download/dotnet/%s"

I know it would be more accurate to say that features in version %s have become part of core F# and are always enabled, but that specifically doesn't apply to ML compatibility. Feel free to change it however you see fit.

[T-Gro]

The wording is good, thanks for adding that 👍 .

I am also thinking about building an unsupported (= not officially shipped) .vsix right before this change and uploading it to people in case they need to maintain such an unsupported codebase. But I can do that JIT when/if the request comes at all.

(reasoning is that pre-change tooling will be better in migrating the codebase, since it will give more accurate warnings)