Skip to content

Conversation

nojaf
Copy link
Contributor

@nojaf nojaf commented Mar 5, 2024

Description

Similar to the extended data added in #15840 I want to know the location of the types (in sig/impl) when an abbreviation is mismatched.

I later want to create a quick fix to update the signature file when necessary:
image

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/releae-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:

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

Copy link
Contributor

github-actions bot commented Mar 5, 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

@nojaf nojaf marked this pull request as ready for review March 5, 2024 15:12
@nojaf nojaf requested a review from a team as a code owner March 5, 2024 15:12
@nojaf
Copy link
Contributor Author

nojaf commented Mar 5, 2024

@DedSec256 would be nice if you could review this PR as well.
Tried to mimic what you did in your work.

Copy link
Contributor

@DedSec256 DedSec256 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! :)
In general, LGTM, there are just few comments

@nojaf
Copy link
Contributor Author

nojaf commented Mar 5, 2024

Thanks for the review @DedSec256!

@psfinaki
Copy link
Member

psfinaki commented Mar 6, 2024

/azp run

Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@psfinaki
Copy link
Member

psfinaki commented Mar 8, 2024

Just to understand, what are the applicable scenarios here?

From my experience (don't want to generalize), type abbreviations are often used for things like type StringList = list<string> whereas here the test is about anon record alias so it took me a while to figure out that this is also technically a type abbreviation and hence the message applies.

I mean I know that it was like this before, just curious if we can be even more specific in the diagnostics here.

@nojaf
Copy link
Contributor Author

nojaf commented Mar 8, 2024

Just to understand, what are the applicable scenarios here?

Basically any type alias:

// sig
type Foo = X

// impl
type Foo = Y

Today, we get a diagnostic that there is a mismatch (X and Y differ). And where in the implementation file it is. What I'm after is getting a pointer to easily locate the type in the signature file.

To perform some code fix like:

image

I can pull this off, but this PR makes it a lot more easy to do it.

@psfinaki
Copy link
Member

psfinaki commented Mar 8, 2024

No I think this is a good move, but then I'd suggest adding more scenarios to the tests.

I mean this below is a very unprecise error, if we can improve the situation it would be awesome.
image

@nojaf
Copy link
Contributor Author

nojaf commented Mar 8, 2024

I'd suggest adding more scenarios to the tests.

The body is irrelevant in this case, so I'm not sure what other scenarios you feel are lacking.

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.

5 participants