Skip to content
This repository has been archived by the owner on Mar 1, 2019. It is now read-only.

Don't take stale defs into account when checking for congruent ones #159

Closed
wants to merge 4 commits into from

Conversation

Xanewok
Copy link
Collaborator

@Xanewok Xanewok commented Nov 29, 2018

We ignored the defs as congruent in cases where we lower the same crate - in the incremental case there's a very high chance our spans are the same as before - however we ignore them as congruent becase those existed previously, but since we did these will not exist now that we lower the data for a given crate.

The approach is to keep a 'queue' of ids we are yet to overwrite and when checking for congruent items don't take into account the defs that are still to be overwritten.

Small repro:

fn other() {}
fn some() {}

When (un)commenting fn some() {} repeatedly we can get stuck in the state where we ignore the fn other() {} before it existed previously. With the invalidated crates queue approach, we first lower the root module and fn other() {} but still ignore it when lowering the data from another crate target (e.g. from test or bin target) as designed previously.
(The test is non-deterministic somehow and requires to coordinate multiple rustc runs, so I didn't add a regression test here - should I?)

Empirically, this seems to have brought back type definitions in goto and hover!
However, I get def already exists at span messages when lowering data from another crate target (in macro case where expansion differs (different gensyms) for both targets but still point to the same source location), but this doesn't seem to have a negative impact on the usability?

@Xanewok
Copy link
Collaborator Author

Xanewok commented Nov 29, 2018

Also this locks the analysis once per loaded crate (cc #157) but it turned out not to be the cause (I originally thought there might be a race condition). It's less locking and seems to be more correct wrt synchronization so I left it 😅

@nrc
Copy link
Member

nrc commented Dec 1, 2018

Merged, minus the locking commit (which we should think about doing in the future).

@nrc nrc closed this Dec 1, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants