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

bugfix: keep evaluated worksheets synced with dependencies in presentation compiler #5248

Merged
merged 1 commit into from Jun 12, 2023

Conversation

kasiaMarek
Copy link
Contributor

@kasiaMarek kasiaMarek commented May 19, 2023

Previously:
If compilers were reloaded (e.g. when doing import build) worksheetsDigests would fall out of sync with dependencies in the presentation compiler.
Now:

  1. Moved worksheetsDigests to compilers so it's reset with presentation compilers too.
  2. Still evaluated worksheets get out of sync w/ pcs, so if there is no pc for worksheet we ask WorksheetProvider to produce one if it has this worksheet evaluated in cache.

should fix: #5199

@kasiaMarek kasiaMarek changed the title bugfix: keep worksheetsDigests synced with dependencies in presentation compiler bugfix: keep evaluated worksheets synced with dependencies in presentation compiler May 22, 2023
@kasiaMarek kasiaMarek changed the title bugfix: keep evaluated worksheets synced with dependencies in presentation compiler bugfix: keep worksheetsDigests synced with dependencies in presentation compiler May 22, 2023
@kasiaMarek kasiaMarek changed the title bugfix: keep worksheetsDigests synced with dependencies in presentation compiler bugfix: keep evaluated worksheets synced with dependencies in presentation compiler May 22, 2023
Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

That is a great catch! The fix is quite clever, but I wonder if we could try to reduce the coupling

sourceDeps.filter(_.toString().endsWith("-sources.jar")),
)
}
maybeRestartWorksheetPresentationCompiler(path, evaluatedWorksheet)
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if instead of getting evaluatedWorksheet we could save the dependency sources together in the Compilers class? This would avoid the bidirectional flow between the two classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I totally agree, the bidirectional flow is terrible here. So when we cancel and clear, we don't clean those saved dependencies? I was just a bit worried that it's a bit counterintuitive to leftover something after cancel. But it's still probably better then what we have now.

The other possibility I was considering was so that WorksheetProvider wouldn't create the pc but Compilers would create a new one for the worksheet if the digest went out of sync between WorksheetProvider and Compilers.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was just a bit worried that it's a bit counterintuitive to leftover something after cancel. But it's still probably better then what we have now.

That's basically a target data (classpath etc.), which we also reuse after restarting, so that's fine.

The other possibility I was considering was so that WorksheetProvider wouldn't create the pc but Compilers would create a new one for the worksheet if the digest went out of sync between WorksheetProvider and Compilers.

If we keep the digest in the Compilers, we should be fine? MAybe in something like WorksheetData that would contain both digest and source jars? And anything more needed to start the presentation compiler?

We would update it and maybe restart the compiler in case of change deps?

Copy link
Member

@jkciesluk jkciesluk left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@tgodzik tgodzik left a comment

Choose a reason for hiding this comment

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

LGTM!

@kasiaMarek kasiaMarek merged commit 1c5faab into scalameta:main Jun 12, 2023
23 checks passed
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.

Worksheet dependency problem with "Go to Definition" in VScode
3 participants