Skip to content

Conversation

@edgarfgp
Copy link
Contributor

@edgarfgp edgarfgp commented Apr 4, 2024

Description

Fixes #1429

Before

image

(Error 358, Line 7, Col 5, Line 9, Col 48, "The override for 'Overloaded: int -> bool' was ambiguous")

After

(Error 358, Line 8, Col 19, Line 8, Col 29, "The override for 'Overloaded: int -> bool' was ambiguous")

Rationale:

  • Show the first ambiguous override instead of the object expression range
  • Only showing the first ambiguous override should be fine as once you add type annotations all the ambiguities will disappear.

Alternative:

  • Show one error for each ambiguous overload which can be noisy.

Checklist

  • Test cases added
  • Release notes entry updated

@github-actions
Copy link
Contributor

github-actions bot commented Apr 4, 2024

❗ 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/8.0.300.md

@edgarfgp edgarfgp marked this pull request as ready for review April 4, 2024 18:19
@edgarfgp edgarfgp requested a review from a team as a code owner April 4, 2024 18:19
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Thanks, it makes things better.

The original issue says that for some reason FSI was reporting different error - did you manage to figure out that discrepancy? What does it show now?

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Apr 5, 2024

The original issue says that for some reason FSI was reporting a different error - did you manage to figure out that discrepancy? What does it show now?

@psfinaki Yeah I tried running this on fsi. As far I can tell it shows the same error. See

image

@edgarfgp edgarfgp closed this Apr 5, 2024
@edgarfgp edgarfgp reopened this Apr 5, 2024
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

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

Alrighty, thanks!

@T-Gro T-Gro enabled auto-merge (squash) April 5, 2024 12:36
@T-Gro
Copy link
Member

T-Gro commented Apr 5, 2024

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

Copy link
Member

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

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

nice, thanks

@edgarfgp
Copy link
Contributor Author

edgarfgp commented Apr 5, 2024

nice, thanks

More coming :) . This was raised by some students in one of our AmplifyingF# Essential lectures. See https://amplifyingfsharp.io/fsharp-essentials/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Improve error reporting: ambiguous override method in object expression

5 participants