Stream: t-compiler

Topic: Attribute validation: check_attr vs codegen_fn_attrs


eddyb (Mar 26 2020 at 10:23, on Zulip):

I think codegen_fn_attrs is just an optimization over checking the attributes elsewhere so validation should probably not be in it

eddyb (Mar 26 2020 at 10:23, on Zulip):

although I could see how that can be hard

Hanna Kruppe (Mar 26 2020 at 10:32, on Zulip):

Well, some of the "validation" in it is directly tied to the actual job of codegen_fn_attrs. For example, codegen_fn_attrs clearly needs to recognize whether an inline attribute is of the form #[inline] or #[inline(always)], and rejecting other forms in the same condition is easier to follow and more robust than duplicating this check in a completely different part of the code (cf. "shotgun parsing"). This seems pretty fundamental.

eddyb (Mar 26 2020 at 10:33, on Zulip):

I think if we wanted something to validate attributes it should be one of those ideas previously suggested but never implemented of having a HIR attribute form that decodes all the builtin attributes

Hanna Kruppe (Mar 26 2020 at 10:36, on Zulip):

So this would be a walk of the entire HIR (as check_attrs tries but fails to be) where every built-in attribute on every item and expression is both validated and turned into a more structured/typed representation?

eddyb (Mar 26 2020 at 10:36, on Zulip):

it would be part of HIR lowering, I assume, and yeah

Hanna Kruppe (Mar 26 2020 at 10:36, on Zulip):

Oh yeah, could be part of lowering.

eddyb (Mar 26 2020 at 10:36, on Zulip):

codegen_fn_attrs's most basic functions would be subsumed by this

eddyb (Mar 26 2020 at 10:37, on Zulip):

however, @anp just added something non-trivial to codegen_fn_attrs so that sort of thing would have to remain its own query

Hanna Kruppe (Mar 26 2020 at 10:37, on Zulip):

should_inherit_track_caller?

eddyb (Mar 26 2020 at 10:37, on Zulip):

(a propagated attribute as opposed to an user-declared one, that is. #[track_caller] from trait methods to impl methods)

eddyb (Mar 26 2020 at 10:37, on Zulip):

yeah that

Hanna Kruppe (Mar 26 2020 at 10:38, on Zulip):

Yeah, having that as a separate query (or part of a query that translated from HIR built-in attributes to the codegen-specific bit flags) seems fine. I just want the validation to be centralized.

eddyb (Mar 26 2020 at 10:39, on Zulip):

I agree, I would be happier with the validation elsewhere and codegen_fn_attrs just gets to delay_span_bug on invalid values (or we can move things to HIR, making it less of a problem, but at the expense of more effort)

eddyb (Mar 26 2020 at 10:40, on Zulip):

I don't actually know how bad it is right now (or if it is bad)

Hanna Kruppe (Mar 26 2020 at 10:41, on Zulip):

The first sounds like we're still duplicating validation logic somewhat, I'd like to avoid that.

Hanna Kruppe (Mar 26 2020 at 10:42, on Zulip):

The actual bugs I currently know of are each simple to hack around, but a more principled solution that fixes all of them + prevents future bugs like that won't be easy in any case. Might as well do the proper thing (move to HIR lowering) if we're going to do it at all.

eddyb (Mar 26 2020 at 10:43, on Zulip):

I guess the code could be shared, so the difference is handling a Result by emitting diagnostics vs just going .unwrap_or_else(|e| { delay_span_bug(e); Default::default() })

eddyb (Mar 26 2020 at 10:43, on Zulip):

something like CodegenFnAttrs::from_hir_attrs

eddyb (Mar 26 2020 at 10:44, on Zulip):

anyway I'm just throwing ideas into the ring, sorry if they're not too useful

Vadim Petrochenkov (Mar 26 2020 at 14:06, on Zulip):

The current attribute checking naturally evolved from checks for individual attributes added by various people.

Vadim Petrochenkov (Mar 26 2020 at 14:08, on Zulip):

We should ideally check all the attribute targets in roughly the same way like attribute inputs were checked in https://github.com/rust-lang/rust/pull/57321.

Vadim Petrochenkov (Mar 26 2020 at 14:12, on Zulip):

enum Annotatable describes all the possible targets, so we can probably put them into the attribute templates (struct AttributeTemplate), more precise checking will probably need to be done for each attribute individually.

Last update: Jun 04 2020 at 18:00UTC