Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add script support to the TransparentCompiler #16627

Merged
merged 15 commits into from
Feb 8, 2024

Conversation

dawedawe
Copy link
Contributor

@dawedawe dawedawe commented Jan 31, 2024

Description

A naive first stab at bringing script support to the TransparentCompiler.

(No release notes needed, I think)

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 Jan 31, 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

… lock, it might be a better approach to just use the one from the BackgroundCompiler
… under a lock, it might be a better approach to just use the one from the BackgroundCompiler"

This reverts commit adfe730.
@dawedawe dawedawe marked this pull request as ready for review February 6, 2024 11:14
@dawedawe dawedawe requested a review from a team as a code owner February 6, 2024 11:14
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.

Looks good!

Can you try to run it in CI with this: b47ba47 (either add here on new branch) ?

It's a bit ghetto way of testing, but actually this PR should resolve the last failures there and then we can add a proper CI leg with experimental features enabled.

@dawedawe
Copy link
Contributor Author

dawedawe commented Feb 6, 2024

Seems like this is causing the last failures:
let tcDependencyFiles = [] // TODO add as a set to TcIntermediate

If we just set it to tcInfo.tcDependencyFiles we make Test project35b Dependency files for ParseAndCheckFileInProject fail because it expects ParseAndCheckFileInProject to NOT include the passed in file itself in the dependency files.

Contrary to that, GetBackgroundCheckResultsForFileInProject (and also ParseAndCheckProject) is expected to have the passed in file in the dependency files by Test project35b Dependency files for GetBackgroundCheckResultsForFileInProject

Is the old behaviour really the one we want to have in future?

@0101
Copy link
Contributor

0101 commented Feb 6, 2024

Hmm, interesting I remember I thought those failures were somehow related to the load closure, have to look at it again.

This DependencyFiles functionality might be partly deprecated by not relying on the file system for source files. Wonder if we should get rid of it altogether. Since the user knows about dependencies they themselves put into the project snapshot. It'd be good to know if/how people use this now...

Anyway, not a blocker for script support I would say, so you can revert the tests and I think we can merge this.

@dawedawe
Copy link
Contributor Author

dawedawe commented Feb 6, 2024

Great, thanks for the help and hints with this one :)

@0101
Copy link
Contributor

0101 commented Feb 6, 2024

Oh, I remember, the test is actually first complaining about notexist.dll not being in dependency files, and in incremental builder it gets there through this:

tcConfigB.knownUnresolvedReferences <- loadClosure.UnresolvedReferences

Which now we also have, but probably are not passing it to the dependency files from there.

Edit: nevermind, it's there in the TcInfo but with all the files as you described. We'll sort this out later.

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 a lot for this!

I didn't put Transparent Compiler into the release notes, since it's experimental and not officially supported, but if you want you can add this.

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

dawedawe commented Feb 6, 2024

I didn't put Transparent Compiler into the release notes, since it's experimental and not officially supported, but if you want you can add this.

Mmh no, I think this PR is just part of the whole story and doesn't need it's own entry.
An entry about the Transparent Compiler (when it's ready) should be enough to also cover this little piece here.

@0101 0101 merged commit ef6d5f2 into dotnet:main Feb 8, 2024
28 of 29 checks passed
@dawedawe dawedawe deleted the transcomp_scriptsupport branch February 8, 2024 12:14
nojaf pushed a commit to nojaf/fsharp that referenced this pull request Feb 28, 2024
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