Stream: t-compiler/rust-analyzer

Topic: Unresolved import diagnostics and incrementality


Jonas Schievink [he/him] (Nov 25 2020 at 14:44, on Zulip):

I'm trying to make the typing_inside_a_function_should_not_invalidate_def_map test fail by putting an unresolved import at the end of lib.rs, but it's still passing.

Since that diagnostic contains a SyntaxNodePtr, I would expect that to fail though. Does anyone know what's going on?

Jonas Schievink [he/him] (Nov 25 2020 at 16:39, on Zulip):

Related issue: I'm trying to make name resolution collect and report errors from macro expansion, but this makes incrementality worse, since every macro call in item position now recomputes the CrateDefMap when changed (since the errors might have changed).

Jonas Schievink [he/him] (Nov 25 2020 at 16:39, on Zulip):

Not sure if that's a big issue, but it seems unfortunate

Florian Diebold (Nov 25 2020 at 16:52, on Zulip):

not sure I understand the problem -- before that, changing the macro call might already have changed the items, which would also require a CrateDefMap recalculation, so what's different?

Florian Diebold (Nov 25 2020 at 16:55, on Zulip):

Since that diagnostic contains a SyntaxNodePtr, I would expect that to fail though. Does anyone know what's going on?

the actual diagnostic returned by the query contains a FileAstId, not a SyntaxNodePtr, which is an index that doesn't change that easily. I think at least that's the answer

Florian Diebold (Nov 25 2020 at 16:56, on Zulip):

it gets turned into a SyntaxNodePtr while collecting diagnostics later

Jonas Schievink [he/him] (Nov 25 2020 at 16:57, on Zulip):

Florian Diebold said:

not sure I understand the problem -- before that, changing the macro call might already have changed the items, which would also require a CrateDefMap recalculation, so what's different?

Previously nameres only fetched the item tree, which doesn't necessarily change if the macro change only affects item bodies

Jonas Schievink [he/him] (Nov 25 2020 at 16:58, on Zulip):

The CrateDefMap stores a list of DefDiagnostics, which contains SyntaxNodePtrs for the UnconfiguredCode case here: https://github.com/rust-analyzer/rust-analyzer/blob/9b512f8569dda32fe2d12114adeed2f612d0190f/crates/hir_def/src/nameres.rs#L303

Florian Diebold (Nov 25 2020 at 16:59, on Zulip):

ok, that might be bad? maybe it should be an AstId, like for the other diagnostics

Florian Diebold (Nov 25 2020 at 17:00, on Zulip):

but you were talking about UnresolvedImport in the beginning, right?

Jonas Schievink [he/him] (Nov 25 2020 at 17:00, on Zulip):

Oh, right, sorry

Jonas Schievink [he/him] (Nov 25 2020 at 17:00, on Zulip):

I meant only UnconfiguredCode

Jonas Schievink [he/him] (Nov 25 2020 at 17:00, on Zulip):

So that explains why I haven't been able to trigger this :D

Florian Diebold (Nov 25 2020 at 17:02, on Zulip):

Jonas Schievink said:

Previously nameres only fetched the item tree, which doesn't necessarily change if the macro change only affects item bodies

ok. If it now just fetches the item tree + errors, that shouldn't have much of an effect on incrementality, right? Only if errors actually change.

It might be possible to collect the errors completely separately though (i.e. nameres uses a projection query that just gets the item tree, without errors, and there's a separate query that collects all the errors from macro expansions. the complicated part would be knowing which macro expansions to query)

Florian Diebold (Nov 25 2020 at 17:05, on Zulip):

If it now fetches the whole expansion + errors, that would be a problem, and there should probably be a query that turns that into item tree + errors

Jonas Schievink [he/him] (Nov 25 2020 at 17:06, on Zulip):

hmm, yeah, that sounds like it should work

Jonas Schievink [he/him] (Nov 25 2020 at 17:06, on Zulip):

thanks!

Jonas Schievink [he/him] (Nov 26 2020 at 19:16, on Zulip):

I ended up going with a simpler firewall query that turns the macro expansion result into just the expansion error https://github.com/rust-analyzer/rust-analyzer/pull/6645

Florian Diebold (Nov 26 2020 at 19:22, on Zulip):

ah yeah, there's no need to combine the two

Last update: Jul 24 2021 at 20:30UTC