Skip to content

Conversation

@nojaf
Copy link
Contributor

@nojaf nojaf commented Feb 12, 2024

Description

Hi @0101,

Trying to add support for type-provider to the transparent compiler.
This is a bit hard to explain so I made a quick recording:

fsharp-type-provider-trabsparent-compiler.mp4

Open questions are:

  • How to unit test?
  • What caches need to be invalidated?

//cc @dawedawe

Fixes # (issue, if applicable)

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:

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

@github-actions
Copy link
Contributor

github-actions bot commented Feb 12, 2024

⚠️ Release notes required, but author opted out

Warning

Author opted out of release notes, check is disabled for this pull request.
cc @dotnet/fsharp-team-msft

@nojaf nojaf added the NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes label Feb 12, 2024
@0101
Copy link
Contributor

0101 commented Feb 12, 2024

That looks good!

One thing to think about: so far we never actually had to manually invalidate any caches, rather everything is driven by constructing the appropriate key for each computation that describes what that computation depends on. I feel it makes reasoning about the whole thing a bit easier. It would be good if we could keep with it, however in this case it might be a bit tricky, so we can probably make an exception.

Wonder if something could be doable inside FSharpProjectSnapshot that would be handling the change event. But ultimately the responsibility would probably have to be shifted to the user of the compiler. So when types are invalidated they would have to re-request with new snapshot (with some type-provider version bumped up).

@nojaf nojaf marked this pull request as ready for review February 13, 2024 14:24
@nojaf nojaf requested a review from a team as a code owner February 13, 2024 14:24
@nojaf
Copy link
Contributor Author

nojaf commented Feb 13, 2024

@0101 I'm really unsure how to write a proper test that would mimic the behaviour of an invalidating type-provider.

This PR has already improved the situation. Could we revisit this maybe some other time?

Copy link
Contributor

@0101 0101 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 moving this forward!

This may not be the ultimate solution but it should hopefully unblock most of basic type provider use cases. We can further improve it in the future.

@psfinaki psfinaki enabled auto-merge (squash) February 13, 2024 14:58
@psfinaki psfinaki merged commit 0e7e490 into dotnet:main Feb 13, 2024
@nojaf nojaf deleted the transparent-compiler-type-providers branch February 13, 2024 16:57
nojaf added a commit to nojaf/fsharp that referenced this pull request Feb 28, 2024
* WIP

* Clear all caches of projectSnapshot

* Revert local test

* Add todo about future plan.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

NO_RELEASE_NOTES Label for pull requests which signals, that user opted-out of providing release notes

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants