icerpc / slicec

The Slice compiler library
Apache License 2.0
13 stars 5 forks source link

Remove include_parent from has_attribute #572

Closed bernardnormier closed 1 year ago

bernardnormier commented 1 year ago

When we check an attribute on a struct, field, operation (etc.), there is no clear use-case for checking if its parent has the same attribute.

So I propose to NOT include this feature in slicec, i.e. remove the include_parent parameter from: https://github.com/icerpc/slicec/blob/bfa9f27523fe60d6a4d6a3bc9f7f8d56b87ad5cd/src/grammar/traits.rs#L55

Naturally, a compiler (say slicec-python) can still lookup a construct and then its parent to check for attributes. It's just this pattern (lookup on construct and its parents recursively) is NOT particularly meaningful.

InsertCreativityHere commented 1 year ago

Naturally, a compiler (say slicec-python) can still lookup a construct and then its parent to check for attributes.

This is not as easy as you make it seem.

Often, functions will take parameters like &dyn Attributable or &dyn Entity. Abstracting the type allows for better code re-use, and is generally considered better design than taking hard-coded concrete types.

But if you look at the API for these traits, there's no functions for navigating to a parent type. 1) It's outside the scope of these traits, since our traits are kept small and focused. 2) Not all Attributable even have a container. For example, SliceFile implements this trait, what would it's parent be?

In C#, this isn't a problem, because you can try to cast your &dyn Attributable to whatever concrete type you want to check, but Rust doesn't natively support downcasting.

Without the functionality provided by this flag, the only way to recursively check for attributes is through accessing, and downcasting the underlying pointers (since our custom smart pointers do support downcasting), but this is a fairly low level operation that really shouldn't be done outside of slicec.


there is no clear use-case for checking if its parent has the same attribute.

I'm pretty sure we're literally using this right now for deprecation checking. Not at the code-gen level, but for slicec itself (slicec will emit warnings if you use deprecated slice types in other places).

I'm also pretty sure it's used for the one attribute that applies to both interfaces and operations, I forget which one it is though.

Of course, we can change these behaviors to whatever we like, but they currently exist, and removing this flag would (or at least should) break them.