Disallow attributes on "type aliases" in parser

[bbannier]

With #1890 we added a validator to reject any attributes on "type alias". To detect such type aliases we used a pretty nasty hack,

spicy/spicy/toolchain/src/compiler/validator.cc

Lines 360 to 363 in 500f285

	// We cannot use `QualifiedType::alias` here since it only works if the
	// declaration aliases some other user-declared type, but would miss
	// alias declarations like `type X = addr;`.
	const bool isAlias = n->type()->print() != n->typeID().str();

We should clean this up by instead disallowing attributes on any declaration which is not a enum or unit (not sure there are more entities on the RHS where even with #1890 we support attributes). A way to do that would be to push the parsing of attributes from type_decl into e.g., unit_type; in fact the parser already has special handling for units,

spicy/spicy/toolchain/src/compiler/parser/parser.yy

Lines 381 to 385 in 500f285

	type_decl : opt_linkage TYPE scoped_id '=' { _docs.emplace_back(driver->docGetAndClear()); }
	qtype opt_attributes ';' { if ( auto u = $6->type()->tryAs<type::Unit>(); u && $7 && *$7 ) {
	u->setAttributes(builder->context(), $7);
	$7 = {}; // don't associate with declaration
	}

With that the added validator would become redundant.

[evantypanski]

Just as a minor point: This would result in an error in parsing that's just "unexpected token" which seems unintuitive when it really looks like these attributes apply to types. There would have to be a little extra handling to give a nice error message, which is exactly what the validator does. I kind of prefer that, but it's not impossible to get a nice error there in parsing. From what I can tell, units have the attributes apply to the type (hence that workaround), but enums have it apply to the decl, so there is another edge case I think?

[bbannier]

Just as a minor point: This would result in an error in parsing that's just "unexpected token" which seems unintuitive when it really looks like these attributes apply to types.

I chatted with @rsmmr about this yesterday and he framed this in a way which made this click for me. Paraphrasing:

• Spicy does not really have a notion of "type alias"
• instead something like type X = ... introduces a binding named X for the thing on RHS
• we can have bindings to existing types, e.g., type Addr = addr; which introduces a new name for addr
• we introduce new types with this syntax as well, e.g., type X = unit {}; declares a type unit {} and binds it to X

#1890 was about disallowing attributes on bindings which are only allowed on declarations (roughly: RHS things with {..}, probably just enum_type and unit_type), but we still allow them in the parser. Usually our parsing is pretty close to what we support (modulo tech debt), but I agree that that can lead to less helpful error messages.

---

More practically, since enum_type (which gets us a type::Enum) currently can neither parse nor store attributes. We could add that, and should then be able to push the parsing into unit_type/enum_type. The type_decl parser for bindings returns us a declaration::Type which still can have attributes, but I think in that way of thinking about bindings/decls this stopped making sense, and we could probably migrate users of that to get attributes out of an underlying Enum/Unit.

[evantypanski]

I know this is possibly a pretty big change, but it's still unintuitive for users that these attributes apply to the unit and enum type and not types generally. A user would see this:

type X = enum { ONE } &cxxname="test";

Then you get the idea that &cxxname adds on to a generic type (well, because it currently does in the grammar). I think that intuition is correct - especially since it wasn't explicitly disallowed for "aliases" (for lack of a better intuitive word :) ) until very recently.

This is confounded because putting an attribute in a field looks extremely close to applying to the type:

type Foo = unit {
    version: uint32 &default=42;
};

I got that from the documentation, which does say this applies to the field, not the type:

provide a default value by adding a &default attribute to the field

So it's not like it's unclear, but I can't help but feel a kind of dissonance between what the syntax looks like and what actually happens, which comes up as multiple real examples of user confusion.

The other attempts to solve this seem like they just kick the can down the road. Even the fix in #1890, which adds a decent error, would be confusing to a new user:

[error] <...>/invalid-type-attributes.spicy:11:17-11:22: attributes are not allowed on type aliases

ok... but attributes are still allowed on types? Well, it kind of looks like it, but only kind of. That would be pretty hard to convey without an outrageously long error or some helpful diagnostic notes.

All that to say, I fully agree that attributes should apply to enum and unit types then remove opt_attributes from the type_decl - as you suggest. But that keeps a weird confusion: attributes still look like they apply to types, it just doesn't make intuitive sense to the user why they wouldn't be allowed in those cases. This is my concern, it just doesn't feel intuitive for the user IMO.

So what if we make it more explicit?:

type X = enum &cxxname="test" { ONE };

type Foo = unit {
    version &default=42: uint32;
};

now it makes a lot more sense to me. Obviously, &cxxname= applies to the enum type. Obviously, &default applies to the version field. This is clear and makes it really obvious that attributes wouldn't apply to "type aliases"

Sorry, this is teetering on off topic for this issue, but this has caused me a lot of confusion and I've already seen multiple instances of this confusing other users, so I thought I'd throw in an extension. This may be better discussed another place, though. I didn't mean to write a wall :)