Stream: t-compiler

Topic: x.py check fails under incremental due to unused attr wei...


pnkfelix (Dec 12 2019 at 15:45, on Zulip):

hmm. is that a sign that the used-attribute architecture needs to change?

eddyb (Dec 12 2019 at 15:45, on Zulip):

maybe my issue isn't beta changing but libstd changing in a way that breaks that incremental-unfriendly attribute tracking

mw (Dec 12 2019 at 15:46, on Zulip):

if it relies on side-effects: yes

pnkfelix (Dec 12 2019 at 15:46, on Zulip):

lets maybe spawn this converation off a parallel topic?

mw (Dec 12 2019 at 15:46, on Zulip):

at least it would need special handling in that case (like diagnostics)

pnkfelix (Dec 12 2019 at 15:46, on Zulip):

for longer term discussion

pnkfelix (Dec 19 2019 at 03:02, on Zulip):

but if used-attributes detection relies on certain queries to run, but incremental skips those queries...

pnkfelix (Dec 19 2019 at 03:03, on Zulip):

its looking more and more to me like the architecture for the unused_attributes lint needs to change

pnkfelix (Dec 19 2019 at 03:04, on Zulip):

we track the set in a field here of a struct that is kept in TLS here. And then each time we need to add to the database or query it, we do a TLS access

pnkfelix (Dec 19 2019 at 03:06, on Zulip):

the data there is not per-cgu though, so we cannot readily save and reload the relevant portion as part of incremental compilation.

pnkfelix (Dec 19 2019 at 03:07, on Zulip):

One thing I'm not clear on is why any rustc_attribute is being flagged as Normal rather than Whitelisted

simulacrum (Dec 19 2019 at 03:09, on Zulip):

it is perhaps worth noting that the mark_used/mark_known attribute piece is something that might be "relatively" easy to remove based on past investigations, replacing by instead storing that information inline inside the Attribute (perhaps in an Arc, but I'm not sure even that is necessary -- I think we basically don't clone Attributes today). We'd need to walk the AST to detect unused attributes, but I think that's not too hard.

I think the ideal might be to move everything past AST construction that looks at attributes to a query that we'd be able to check for being executed (e.g., has "attribute(AttrId)" run? if so, it's been used). This is a pretty invasive change though

simulacrum (Dec 19 2019 at 03:10, on Zulip):

One thing I'm not clear on is why any rustc_attribute is being flagged as Normal rather than Whitelisted

I don't think this matters ultimately, though it may point at an immediate fix. But my impression that the problem here is one that we could see in any Rust project, not just libstd/libcore, it's not a matter of rustc attributes or other types of attributes.

pnkfelix (Dec 19 2019 at 03:11, on Zulip):

my impression that the problem here is one that we could see in any Rust project

I thought that too, but I have had difficulty constructing a case that didn't use some kind of rustc_attribute.

simulacrum (Dec 19 2019 at 03:11, on Zulip):

(And IIRC, Normal just means that we might emit unused errors, which seems correct for any attribute, right? Regardless of it being rustc-specific?)

pnkfelix (Dec 19 2019 at 03:12, on Zulip):

I'll look into the allow_internal_unstable example provided on #64764 though

simulacrum (Dec 19 2019 at 03:12, on Zulip):

I think it might be that we are far more eager about using non-rustc-specific attributes, i.e., we're almost guaranteed to mark them as used regardless of which queries run

simulacrum (Dec 19 2019 at 03:12, on Zulip):

this is very much not true of at least some rustc attributes though I imagine (since they're only visited by e.g. typeck or so)

pnkfelix (Dec 19 2019 at 03:13, on Zulip):

(And IIRC, Normal just means that we might emit unused errors, which seems correct for any attribute, right? Regardless of it being rustc-specific?)

Right; but it seems less risky to omit fail to emit unused errors for rustc_attributes given the fragility of the architecture, at least for these cases.

pnkfelix (Dec 19 2019 at 03:14, on Zulip):

I think it might be that we are far more eager about using non-rustc-specific attributes, i.e., we're almost guaranteed to mark them as used regardless of which queries run

yes this matches my own running hypothesis about my failure to replicate

simulacrum (Dec 19 2019 at 03:20, on Zulip):

Sure -- I agree that we might be able to patch over the problem by whitelisting rustc-specific attrs for now

pnkfelix (Dec 19 2019 at 03:22, on Zulip):

but then at that point, is that buying us anything vs the approach taken in #67101 of just doing #[allow(unused_attributes)] at the relevant module(s) ?

pnkfelix (Dec 19 2019 at 03:23, on Zulip):

(it would buy us something if we were aware of non-rustc_attribute cases that unused_attributes readily catches. I guess custom attributes were the original impetus...)

simulacrum (Dec 19 2019 at 03:24, on Zulip):

I think no.

simulacrum (Dec 19 2019 at 03:24, on Zulip):

I would personally not be entirely against just giving up on unused attributes entirely for now

simulacrum (Dec 19 2019 at 03:25, on Zulip):

Since it seems like in almost all cases if we're not using it we don't really want that to be a lint

simulacrum (Dec 19 2019 at 03:25, on Zulip):

i.e., it's either a future compat hazard if we do start using it or is just a questionable choice to accept it

pnkfelix (Dec 19 2019 at 03:49, on Zulip):

okay, yeah, I reviewed the history a bit in RFC 572. I think we have reached the point where the unused_attribute lint is indeed not buying us anything at all.

pnkfelix (Dec 19 2019 at 03:50, on Zulip):

and therefore I too would be okay with the idea of just throwing out that functionality.

pnkfelix (Dec 19 2019 at 03:59, on Zulip):

though I should double-check behavior on syntax extensions that leverage secondary attributes as a way to signal info from within the input syntax to the macro

Zoxc (Dec 19 2019 at 05:37, on Zulip):

I'd like to just explicitly list which attributes are allowed and where.

simulacrum (Dec 19 2019 at 13:09, on Zulip):

@Zoxc I think that essentially mirrors what I said? Unless I'm misunderstanding -- though we could also not do that yet, but still delete the unused attributes lint.

Last update: Jan 21 2020 at 08:20UTC