Stream: t-compiler/rust-analyzer

Topic: Extending #[cfg] diagnostics to bodies


Jonas Schievink [he/him] (Oct 23 2020 at 14:04, on Zulip):

Now that we diagnose inactive #[cfg]s on the item level, I'd like to extend this to bodies. One issue with this is that we cannot associate the diagnostic with the post-lowering ExprId etc., because those don't exist for unconfigured expressions/fields/etc., so this can't work like type check diagnostics. And storing SyntaxNodePtrs in the Body would invalidate it on every change that happens before the body in its file (right?).

Would it be a good idea to instead collect these diagnostics in the BodySourceMap? This already contains SyntaxNodePtrs so it shouldn't affect caching.

Jonas Schievink [he/him] (Oct 23 2020 at 14:06, on Zulip):

Now that I think about it, I've added SyntaxNodePtrs to CrateDefMap for unconfigured items. That should probably be changed. I think I couldn't use AstId because I wanted something that erased the type of AST node, but that might be unnecessary.

matklad (Oct 23 2020 at 14:49, on Zulip):

AstId stores something erased interally

matklad (Oct 23 2020 at 14:50, on Zulip):

For cfged-out expressions, perhaps we need just a special kind of expression? Disable { what: Expr}?

matklad (Oct 23 2020 at 14:50, on Zulip):

hm, probably not the expression , but statement

Jonas Schievink [he/him] (Oct 23 2020 at 15:01, on Zulip):

matklad said:

AstId stores something erased interally

Yeah, I think I ignored that because it's not pub

Jonas Schievink [he/him] (Oct 23 2020 at 15:02, on Zulip):

matklad said:

hm, probably not the expression , but statement

The problem is that #[cfg]s can be on a lot more than just statements or expressions

Jonas Schievink [he/him] (Oct 23 2020 at 15:02, on Zulip):

It can be on fields and function call arguments for example

Jonas Schievink [he/him] (Oct 23 2020 at 15:02, on Zulip):

And on match arms

matklad (Oct 23 2020 at 15:03, on Zulip):

Riiight

Jonas Schievink [he/him] (Oct 23 2020 at 15:03, on Zulip):

It doesn't really sound very clean to add special cases to all of those just for one diagnostic

matklad (Oct 23 2020 at 15:04, on Zulip):

Well, I imagine that, if we have semantic model for disabled things, we can also provide best effort completion, for example

matklad (Oct 23 2020 at 15:04, on Zulip):

but yeah, this fundamentally goes against the grain of the language

Jonas Schievink [he/him] (Oct 23 2020 at 15:04, on Zulip):

true

matklad (Oct 23 2020 at 15:04, on Zulip):

so, it probably doesn't make sense to try to handle that at this stage

matklad (Oct 23 2020 at 15:05, on Zulip):

Yeah, I guess we should just stuff the info in the SourceMap

Jonas Schievink [he/him] (Oct 23 2020 at 15:05, on Zulip):

We should probably also have some more incrementality tests for this sort of stuff, since otherwise we'd almost never notice these subtle problems (ItemTree is also missing these)

matklad (Oct 23 2020 at 15:22, on Zulip):

It's kinda hard to make a general incremental tests here though, which would trigger when we make unexpected changes.

Jonas Schievink [he/him] (Oct 23 2020 at 15:29, on Zulip):

Yeah, it's a bit tricky. But something like "prepending a newline to this file doesn't invalidate item bodies" seems like a useful thing to test for.

Jonas Schievink [he/him] (Oct 23 2020 at 15:29, on Zulip):

If something does break a test like that at least we won't accidentally regress it

Jonas Schievink [he/him] (Oct 23 2020 at 15:30, on Zulip):

For the item tree this seems fairly useful, because its entire point is to avoid reparsing stuff, so that should be easy-ish to check

matklad (Oct 23 2020 at 15:31, on Zulip):

Item tree is indirectly checked by nameres

Jonas Schievink [he/him] (Oct 23 2020 at 15:31, on Zulip):

Oh, did we add a test for that?

matklad (Oct 23 2020 at 15:33, on Zulip):

typing_inside_a_macro_should_not_invalidate_def_map

Jonas Schievink [he/him] (Oct 23 2020 at 15:34, on Zulip):

Yeah, although that predates ItemTree

Jonas Schievink [he/him] (Oct 23 2020 at 15:34, on Zulip):

It doesn't check that the ItemTree actually avoids parsing the source file twice

matklad (Oct 23 2020 at 15:35, on Zulip):

"Parsing twice" in a sense "not redo the same work for impls" is not something this test is intended to test

matklad (Oct 23 2020 at 15:36, on Zulip):

"Parsing twice" is re-parsing after source code change I think is inevitable?

matklad (Oct 23 2020 at 15:36, on Zulip):

That's sort of how item tree works -- we re-parse and rebuild it, but we stop there, a the tree is not changed

Jonas Schievink [he/him] (Oct 23 2020 at 15:37, on Zulip):

Yeah, but previously we'd reparse the input twice, once for nameres, and once for item data queries

Jonas Schievink [he/him] (Oct 23 2020 at 15:38, on Zulip):

I suppose it would not really be testing incrementality

matklad (Oct 23 2020 at 15:40, on Zulip):

correct, it'll be testing LRU capacity

matklad (Oct 23 2020 at 15:40, on Zulip):

Though, yeah, I guess we can actully test this

matklad (Oct 23 2020 at 15:40, on Zulip):

At least in theory, something like typing_inside_function_body_doesn't recompute crate_impls

Last update: Jul 28 2021 at 04:00UTC