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

Do not update externs/cache-db when codgenTargets is empty #4178

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

wclr
Copy link
Contributor

@wclr wclr commented Aug 26, 2021

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's externs.cbor file, and doesn't update cache-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 update cache-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:

  • Added a file to CHANGELOG.d for this PR (see CHANGELOG.d/README.md)
  • Added myself to CONTRIBUTORS.md (if this is my first contribution)
  • Linked any existing issues or proposals that this pull request should close
  • Updated or added relevant documentation
  • Added a test for the contribution (if applicable)

@wclr wclr changed the title Do not modify externs/cache-db when codgenTargets is empty Do not update externs/cache-db when codgenTargets is empty Aug 26, 2021
@hdgarrood
Copy link
Contributor

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.

This actual meta update also triggers rebuilding of downstream modules while a consequent full rebuild, which is not needed.

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.

@nwolverson
Copy link
Contributor

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’m not sure about this. At the moment, rebuilding downstream modules is always required after an upstream module is changed

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

  1. An explicit way of asking purs ide for a diagnostics-only single module compile (which might be used "as you type" rather than the current which works as you save)
  2. As an interim measure purs ide could avoid saving bitwise identical output, if indeed that is an issue, this is surely far easier than the full cut-off question

@hdgarrood
Copy link
Contributor

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.

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.

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.

@nwolverson
Copy link
Contributor

Why not save a module's new externs if we've already computed them?

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

@hdgarrood
Copy link
Contributor

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.

@wclr wclr changed the title Do not update externs/cache-db when codgenTargets is empty Allow ide module rebuilds without touching fs output/cache-db Jul 4, 2022
@wclr wclr changed the title Allow ide module rebuilds without touching fs output/cache-db Do not update externs/cache-db when codgenTargets is empty Jul 4, 2022
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

3 participants