Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Rust: introduce typed labels #17460

Merged
merged 5 commits into from
Sep 16, 2024
Merged

Rust: introduce typed labels #17460

merged 5 commits into from
Sep 16, 2024

Conversation

redsun82
Copy link
Contributor

@redsun82 redsun82 commented Sep 13, 2024

This introduces a generate {{x}}TrapLabel wrapper around untyped labels for each class in the schema (including class that are inherited from). Then all relevant From traits are implemented so that .into() can be used to do "casting" of labels, with the compiler checking that the cast is valid.

Also, this adds automatic type-based prefixing of global trap keys.

@github-actions github-actions bot added the Rust Pull requests that update Rust code label Sep 13, 2024
args,
ellipsis_index,
})
self.trap
Copy link
Contributor

@aibaars aibaars Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't we avoid all the into() calls. They are a bit annoying to write and make things less type safe. Would it be possible to have the following type for trap.emit() ?

fn emit<T>(node: T) -> Label<T>

That way the type checker knows that emit(generated::TuplePat{}) returns a Label<generated::TuplePat>

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's what's actually happening already (with T::Label rather than Label<T> because I encountered some problems with the latter). So emit gives the very precise type. The problem is that then we need to convert that to a more generic label, for example from a TuplePat::Label to a Pat::Label, which from the TRAP point of view is perfectly valid. As rust abhors implicit conversions, in that case an into() must be used.

However, that doesn't mean it's unsafe, as an into() from X::Label to Y::Label will only compile if X is a subclass of Y.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, it feels a bit more complicated then what I would do in Haskell, but it should indeed work ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll still play around with it for a bit, maybe I'll get to do a more ergonomic Label<T>. But in any case because it's Rust we won't get rid of the into()s.

Copy link
Contributor Author

@redsun82 redsun82 Sep 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I managed to get it into a much better spot, using Label<T> (the problem was the missing PhantomData and the wrong derived Clone and Copy). And, I ended up having an explicitly unsafe from_untyped, which I think is pretty neat to prevent accidental use :). The clippy check also enforced a safety comment on unsafe blocks, which is also pretty cool

Base automatically changed from redsun82/rust-std-files to main September 13, 2024 14:23
{{! virtual class, make it unbuildable }}
#[derive(Debug)]
pub struct {{name}} {
_unused: ()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't this just be pub struct {{name}} { } ?

Copy link
Contributor Author

@redsun82 redsun82 Sep 16, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

a pub struct X {} can be built by a user (e.g. let x = X{}), and I'd like to avoid that as we shouldn't be building instances of structs that are not leaves in the hierarchy. By adding one private field we disallow that.

@redsun82 redsun82 marked this pull request as ready for review September 16, 2024 10:04
@redsun82 redsun82 requested a review from a team as a code owner September 16, 2024 10:04
@aibaars aibaars added the no-change-note-required This PR does not need a change note label Sep 16, 2024
aibaars
aibaars previously approved these changes Sep 16, 2024
@aibaars
Copy link
Contributor

aibaars commented Sep 16, 2024

This looks great, thanks!

@redsun82 redsun82 merged commit d24d933 into main Sep 16, 2024
21 of 24 checks passed
@redsun82 redsun82 deleted the redsun82/rust-typed-labels branch September 16, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-change-note-required This PR does not need a change note Rust Pull requests that update Rust code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants