Remove ML compatibility

[kerams]

Description

Implements fsharp/fslang-suggestions#1407

• Released keywords asr, land, lor, lsl, lsr, lxor (mod continues to be reserved)
• Hidden or and & operators in FSharp.Core for binary compatibility
• Removed support for .ml and .mli source files
• Removed #light and #indent directives (they are now a no-op, combined with "off" they give an error)
• 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 ML specific warning or error (the only exception is parenthesized dot get, which gives a construct removed 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

• Test cases added

• 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]

❗ Release notes required

---

✅ 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

[auduchinok]

Thanks for this @kerams!

What do you think about keeping the parser rules and just always producing the errors? We could easily remove them in future if wanted to repurpose these constructs in the language. For now keeping them would allow to parse older code which could be nice as long as it doesn't need maintenance in the parser.

[auduchinok]

Let's just always produce this error instead of removing the parser rule and failing to parse a possibly old file?

[T-Gro]

The compiler will already error on seeing a deprecated language version value, prior to starting parsing.

[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.)

[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.

[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 #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)

[T-Gro]

This contribution simplifies the compiler codebase, especially the lexer and parser.

Big thank you to @kerams for starting the activity of stabilizing the "core of F# language" by reducing the number of supported versions, and taking the largest language feature by far!

I added a few minor comments only.

[T-Gro]

The compiler will already error on seeing a deprecated language version value, prior to starting parsing.

[T-Gro]

Can the atomTypeNonAtomicDeprecated be fully removed (incl. this file's header) and inlined with atomType?

[T-Gro]

Can you please do a final "62" full text search?
This file and the fslex files still have it, among others.

I have also not seen a removal of the message behind "62", which I found strange.. ?

[kerams]

This was removed

fsharp/src/Compiler/Facilities/DiagnosticsLogger.fs

Lines 613 to 617 in 0b836fb

	let mlCompatWarning s m =
	warning (UserCompilerMessage(FSComp.SR.mlCompatMessage s, 62, m))
	let mlCompatError s m =
	errorR (UserCompilerMessage(FSComp.SR.mlCompatError s, 62, m))

including fscomp texts

[T-Gro]

The change deserves a lift in Versions.props for the <FCS.. version, FCS consumers will need to accommodate.
(does not belong to this file, but tooling is failing me horribly on this PR. I lost all comments already :(( )

[kerams]

Will these changes also automatically flow to some minor update for .NET 10? That would make the current out of support error message misleading. (which is why I asked whether the changelog belongs in the 10.0.200 markdown files and not vnext)

[T-Gro]

As of now it would still go to the 10.0.200 release (early next year) unless we wait a little and make it skip that and go into the first preview of net11 instead.

[T-Gro]

But no matter which of the two it is (10.0.200 or a v11 early preview), 10.0.10x SDK band will not get this. 10.0.10x will remain the LTS supported version with capability for compiling code with ML syntax.