Don't use typecheck rule for non FOIs in refine imports plugin #2995
Conversation
| -- We should only call the typecheck rule for files of interest. | ||
| -- Keeping typechecked modules in memory for other files is | ||
| -- very expensive. | ||
| assert (foi /= NotFOI) $ typeCheckRuleDefinition hsc pm |
There was a problem hiding this comment.
I have this assertion locally and it often fails for me but not necessarily in bad ways, just seems there is a race?
There was a problem hiding this comment.
Can you get a call stack?
There was a problem hiding this comment.
If you close a file while a typecheck has been queued, then on restart that typecheck will be executed, but the assertion will fail because it is not an FOI any longer.
I think this is harmless because it should be garbage collected soon after?
There was a problem hiding this comment.
Sounds like a good reason to make it just a warning :)
There was a problem hiding this comment.
Yes, but I think the false positive rate can be very high to the point of being annoying. Maybe we should just silently log it, with a message that explains this caveat and asks for a bug report.
Something to effect of "Warning: Typecheck executed for file that is not currently open. This is harmless if this file was recently open and has now been closed. If not, please report a bug."
There was a problem hiding this comment.
If you close a file while a typecheck has been queued,
Can the check be moved to the code that schedules the typecheck?
| -- We should only call the typecheck rule for files of interest. | ||
| -- Keeping typechecked modules in memory for other files is | ||
| -- very expensive. | ||
| assert (foi /= NotFOI) $ typeCheckRuleDefinition hsc pm |
There was a problem hiding this comment.
This is pretty aggressive, right? It'll throw, and presumably crash HLS? Couldn't we just log the occurrence?
There was a problem hiding this comment.
Assertions are disabled by -O. So this assert won't trigger for users, and I doubt it will even trigger in CI
There was a problem hiding this comment.
Okay, then I think we should definitely log too! I would like to know if this is happening in general usage as well. A warning log would be fine.
|
Ah, the same mysterious ghcide windows failure that I was getting! That makes me feel much better that it's not actually my fault 😂 Would be nice if we had any idea what's going on there though. |
|
@wz1000 add a log and I think we can merge this? |
wz1000
force-pushed
the
wip/foi-no-typecheck
branch
2 times, most recently
from
b710651
to
a3d45f4
Compare
Also add an assertion to check that we never use non-FOI rules Fixes #2962
wz1000
force-pushed
the
wip/foi-no-typecheck
branch
from
a3d45f4
to
3ad89f5
Compare
Also add an assertion to check that we never use non-FOI rules
Fixes #2962