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

[WIP] Incremental builder re-write #8040

Closed

Conversation

TIHan
Copy link
Member

@TIHan TIHan commented Dec 24, 2019

This basically re-writes the incremental builder while keeping the same API. This gets rid of the internal DSL framework that used to control it.

Reasons why we want to do this:

  • Fine grain control of the build
    • Will help us add/change APIs that can ask very specific questions about the build, i. e. If symbol caching is disabled, find a symbol in a file - would need to re-type-check the file.
  • Easier to read/understand (hopefully!) and debug

I understand why the framework was built, but we need to move forward with something that gives us total control rather than force us into a reactive structure.

  • Manual Testing - need to make sure the build is not constantly being re-evaluated
  • Review time stamp logic
  • Fix up calls in typeCheck that are not tail-recursive
  • Cleanup

src/fsharp/service/IncrementalBuild.fs Outdated Show resolved Hide resolved
src/fsharp/service/IncrementalBuild.fs Outdated Show resolved Hide resolved
src/fsharp/service/IncrementalBuild.fs Outdated Show resolved Hide resolved
src/fsharp/service/IncrementalBuild.fs Show resolved Hide resolved
src/fsharp/service/IncrementalBuild.fs Show resolved Hide resolved
@KevinRansom
Copy link
Member

Wow, lots of code was removed. It looks great, I think we are going to have to sit down and go over how we don't need what was removed.

@TIHan
Copy link
Member Author

TIHan commented Dec 30, 2019

This is almost done. I need to spend time to review the time stamp logic.

src/fsharp/service/IncrementalBuild.fs Outdated Show resolved Hide resolved
src/fsharp/service/IncrementalBuild.fs Outdated Show resolved Hide resolved
src/fsharp/service/IncrementalBuild.fs Show resolved Hide resolved
@TIHan TIHan changed the title Incremental builder re-write [WIP] Incremental builder re-write Jan 17, 2020
@TIHan
Copy link
Member Author

TIHan commented Jan 17, 2020

I'm going to hold off of this for a bit. There might be a better way to move tooling in a direction that doesn't require re-writing this at the moment.

@cartermp
Copy link
Contributor

Closing out old experiment

@cartermp cartermp closed this Jun 27, 2020
@auduchinok
Copy link
Member

@TIHan @cartermp Could you tell more about other ways you're thinking about? Why this change isn't considered good to continue? The initial goals look very welcome.

@cartermp
Copy link
Contributor

The goals are great - this (and other closures) are more about clearing out the queue of PRs than anything else. There's value in each of these experiments, but they come with some tradeoffs that will take time to consider, and the PRs as they stand today wouldn't be something we'd pull in without that consideration, and without ample time to validate the sweeping changes made across a variety of codebases.

One prerequisite is establishing a sound way to turn on/off experimental features in VS tooling that live in the compiler layer. Doing a wholesale rewrite of the incremental builder without a way to revert back would not be something we'd do, especially consider the every-growing set of existing codebases that need to be stable in VS tooling.

I think this is something we'll ultimately do though, since it eliminates so much complexity. Perhaps after F# 5 winds down it can be considered again.

@dsyme
Copy link
Contributor

dsyme commented Oct 27, 2020

@TIHan I looked over this and it looks good on first sight. Do you think you'll return to it?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants