-
Notifications
You must be signed in to change notification settings - Fork 3.1k
PC invalidates previous work on new run [ci: last-only] #10867
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
base: 2.13.x
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good / safe to me, I don't fully follow (what are the "parser callbacks"?)
@@ -301,6 +301,7 @@ class Global(settings: Settings, _reporter: Reporter, projectName: String = "") | |||
def isOutOfDate: Boolean = outOfDate | |||
|
|||
def demandNewCompilerRun() = { | |||
if (!lastWasReload) allSources.foreach(getUnit(_).foreach(reset(_))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would it be equivalent to add a boolean param to demandNewCompilerRun
and set it true at the callsite in reload
?
There is |
BTW I was curious (or annoyed) about the test; it's not my intention to muck with (or break) the PC. I assumed incorrectly that it would be a mechanism problem that required no special knowledge of PC semantics and operations. |
27a4ae7
to
11a33e4
Compare
I was following the "outdated symbols in the IDE" comment to
which is why that doesn't drop on failed lookup, only on successful lookup of the wrong (shadowing) symbol. (I was recently reminded to look again at the 2.13.15 PR to warn about "phantom object" from the classpath: the class got a module, so the same module defined in source is hidden; the method of detection is to look in the package owner.) The test in The change in
8000
|
I changed that counter to 1000 and ran ok, then back to 2 and it
for
That is the infamous flaky half of the Edit: just noticed that I'm pretty sure Homer Simpson works with booleans at the power plant. He may even be a "boolean specialist". |
I think the test is just taunting me, trying to draw me out into the open ocean, like a scene from Moby Dick. Maybe there is a reason I called the second test |
11a33e4
to
4bbd250
Compare
Before and after log for the "sequential parsing" change.
|
Never mind. 10K rep did not complete on Windows 10
I'll check the other two ideas (brittle wait and not using nanoTime). |
The
getDocComment
command creates anewTyperRun
as of #2387. ThencheckForMoreWork
notices there is a new run afterpollForWork
and then willdemandNewCompilerRun
.However, a second execution of the test would fail because the unit is deemed up-to-date. This commit updates the
minRunId
which governs whether the unit is stale when the run is created.I verified that the test would always fail the second time under Linux but occasionally the first time under Windows.
I don't see a race condition, but the code uses
currentTimeMillis
instead ofnanoTime
for timeouts, so maybe it's a Windows clock issue. Or maybe a timing issue (what the PC thread completes before the next ask) causes a different code path. (Two guesses.)The test comment somewhere says "check you can get a comment one time". The original PR was #2156.
My previous fix was to
reset
all sources atdemandNewCompilerRun
, which would likely be heavy-handed.Probably the change atdemandNewCompilerRun
can be refined, as it matters just for doc comments.askParse
is tweaked to submit one file at a time, since otherwise the parser callback on the first job will look for work and start the next parse. (In this test, the chain stopped there because the second file was trivial and invoked no callback.)Fixes scala/scala-dev#840