Better ranges for implicit inherit error reporting.

[edgarfgp]

Description

Better ranges for implicit Inherit error reporting.

Before

After

type ClassA() = class end

type ClassB() = class end

type ClassC() = class end

type Class() =
    inherit ClassA()
    inherit ClassB()
            ^^^^^^
    inherit ClassC()
            ^^^^^^

Checklist

• Test cases added
• Release notes entry updated

[github-actions]

❗ Release notes required

---

✅ Found changes and release notes in following paths:

Change path	Release notes path	Description
src/Compiler	docs/release-notes/.FSharp.Compiler.Service/9.0.200.md

[psfinaki]

Thanks Edgar!

[abonie]

Do we actually want both these errors? I mean 959 and 932 for each additional inherit?

Is there ever a case where there's going to be a 932 without a corresponding 959? If not, is 932 actually useful?

And if we do want both, I'd argue 932 should be thrown only once regardless of how many extra inherit specifications there are and its range should actually be line 8 in this example above, so like it was before:

type Class() =
     ^^^^^

[edgarfgp]

@abonie

Do we actually want both these errors? I mean 959 and 932 for each additional inherit?

I guess arguably we do not need 959 as 932 will cover the cases for having multiple inherit. Should we get ride of 959 ?

Personally I think showing the specific ranges for inherit is more clear and guides the user toward removing rather than just using the type definition.

[edgarfgp]

Please wait for https://github.com/dotnet/fsharp/pull/17905/files to be merged first.

[psfinaki]

Marker to not merge this accidentally.

[auduchinok]

@edgarfgp Thanks for your contribution! I'm sorry to say this, but I disagree with this change. I'll try to explain it on this example:

type T() =
    inherit A()
    inherit B()

I expect that there's an error about multiple inherit members. That means that the actual source of the errors are the inherit members themselves, not the type names inside of them. Moving the error to the inherited type name may say to many people that there's no error with the inherit member itself, and there's something wrong with the inherited type usage.

[auduchinok]

Could you please also add tests for multiple base interfaces, if there're none?

type I =
    inherit IA
    inherit IB

[edgarfgp]

@edgarfgp Thanks for your contribution! I'm sorry to say this, but I disagree with this change. I'll try to explain it on this example:

type T() =
    inherit A()
    inherit B()

I expect that there's an error about multiple inherit members. That means that the actual source of the errors are the inherit members themselves, not the type names inside of them. Moving the error to the inherited type name may say to many people that there's no error with the inherit member itself, and there's something wrong with the inherited type usage.

@auduchinok No need to apologise for disagreeing. This is a collaborative effort and Im glad that you are reviewing my PRs :)

So is the following what you would expect ?

[<Fact>]
    let ``Types cannot inherit from multiple concrete types.`` () =
        Fsx """
type ClassA() = class end

type ClassB() = class end

type ClassC() = class end

type Class() =
    inherit ClassA()
    inherit ClassB()
    inherit ClassC()
        """
        |> typecheck
        |> shouldFail
        |> withDiagnostics [
            (Error 959, Line 10, Col 5, Line 10, Col 12, "Type definitions may only have one 'inherit' specification and it must be the first declaration")
            (Error 959, Line 11, Col 5, Line 11, Col 12, "Type definitions may only have one 'inherit' specification and it must be the first declaration")
            (Error 932, Line 10, Col 13, Line 10, Col 19, "Types cannot inherit from multiple concrete types")
            (Error 932, Line 11, Col 13, Line 11, Col 19, "Types cannot inherit from multiple concrete types")
        ]

[auduchinok]

@edgarfgp More or less, yes. I'd probably prefer the 932 errors to be on the Class type name, not on the inherit members.

[edgarfgp]

@edgarfgp More or less, yes. I'd probably prefer the 932 errors to be on the Class type name, not on the inherit members.

Humm. But using the type name and not the inherit range makes it look like multiple inherits are allowed(Unless you hover the type name). I would prefer to be use the accurate range.

[auduchinok]

Humm. But using the type name and not the inherit range makes it look like multiple inherits are allowed

Hm, it should not, since 959 errors are still going to be on the inherit members.