-
Notifications
You must be signed in to change notification settings - Fork 562
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
Do not update externs/cache-db when codgenTargets is empty #4178
base: master
Are you sure you want to change the base?
Conversation
I think this is not ideal because if you’re invoking the compiler with no codegen targets in order to learn whether something type checks, you’re not going to be able to benefit from incremental builds at all. For example, if I change module A and then run the compiler with no targets, and then change B (which imports A) and rerun the compiler with no targets, I’ll have to type check A again even though I ought to already know that it type checks.
I’m not sure about this. At the moment, rebuilding downstream modules is always required after an upstream module is changed. We may be able to change this with #3724 but that’s going to take a bit of work. |
I think there are 2 separate cases and this should probably be something more explicit - psc-ide updating externs (and internal state) after rebuilding 1 file feeds into type information available when working on another file, makes sense regardless of whether e.g. js or corefn is output. On the other hand there is a use case for invoking the compiler to get error information, eg feedback as you type (without saving a file), and it seems that should not be a stateful operation. One could imagine more tailored feedback on files that only partially compile, but just the current fast-rebuild workflow is already useful here, apart from the issues discussed.
I believe the issue is that even if the module does not change downstream modules are rebuilt. This is a case where an incremental build would not have built that module, but it is requested via the IDE server, "poisoning" the next incremental build So I would propose
|
Ok, thanks for the additional context. Just so you know, I am hesitant to approve this, because extreme care needs to be taken over this part of the compiler; it's incredibly easy to invalidate an important assumption by accident and end up having the compiler claim that build products are up to date when they aren't, which would be a very serious bug.
It's this part that I'm not sold on. Why not save a module's new externs if we've already computed them? It seems quite plausible to me that after making changes to some module (while requesting fast feedback), you might want to make changes to some other module which is downstream of it, in which case you will want the externs to have been saved because otherwise the compiler would have to either typecheck the upstream module again every time, somewhat defeating the point of fast feedback, or incorrectly using the old externs (in which case you get nonsense answers). In the case of the IDE rebuild saving externs when they haven't changed and updating the timestamp, I think we should at least investigate whether we could have the IDE detect that there's no need to rebuild and not do it in the first place. But I'd much rather just work towards proper cutoff; that's going to help everyone, and I suspect it'll be simpler overall. |
I think this way lies truly incomprehensible behaviour, in the presence of any automatic "as you type" rebuild. If you start updating externs with in-progress file contents that have never yet been saved to disk, your regular builds are going to be just wrong, for example. I also don't think your suggested workflow makes sense. I would argue that I don't expect edits in one file to affect another file unless I save it - but maybe I've not been exposed to a sufficiently magic IDE that allows one to actually forget about the file system. Fortunately I'm coming to realise there's a simple workaround that would allow us to safely experiment with this behaviour without compiler changes - if we just submit any rebuild requests that are intended to be "non-binding" under a temporary placeholder module, then they cannot possibly affect the state of the regular build process |
Oh right sorry, I see, I had misunderstood you. I do agree that there’s no reason to save externs for a version of a file which hasn’t been written to the disk yet. The temporary placeholder module workaround seems reasonable to me. |
23aad9c
to
dcdd440
Compare
Description of the change
This PR introduces a check while ide rebuilds and make processes so that if
codegenTargets
set is empty it doesn't touch the compiled output at all: doesn't modify the module'sexterns.cbor
file, and doesn't updatecache-db.json
. This would allow the implementation of getting fully pure diagnostics for unsaved changes or just opened files by an editor tooling.Currently, if purs-ide is asked to rebuild a module with a temporary file and empty codegen targets it still will update
extern.cbor
file for the module and updatecache-db
entry even though no actual compilation output was produced and the actual source module was not changed on the disk. This seems to introduce an inconsistency in the actual (filesystem-related) state, and more correct for this rebuild case just not to touch anything on the filesystem. This actual meta update also triggers rebuilding of downstream modules while a consequent full rebuild, which is not needed.I've compiled it and tested it in the described scenario of implementation of diagnostics on typing/file opening, and it works as I expect it to work. All existing tests are passing. I haven't added tests of this, because it should be reviewed and approved first. There may exist some scenarios that I miss, where the proposed change may introduce problems, also considering that it relates not just to ide rebuilds.
Related PRs:
#3789
Checklist: