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

Merge Class and Exception grammar elements #697

Open
bernardnormier opened this issue Jul 30, 2024 · 1 comment
Open

Merge Class and Exception grammar elements #697

bernardnormier opened this issue Jul 30, 2024 · 1 comment
Labels
enhancement New feature or request proposal
Milestone

Comments

@bernardnormier
Copy link
Member

Class and Exception are very similar:
https://github.com/icerpc/slicec/blob/main/src/grammar/elements/class.rs
https://github.com/icerpc/slicec/blob/main/src/grammar/elements/exception.rs

And the mapping for both (in C# for now) is also extremely close (see # icerpc/icerpc-csharp#4024).

It would be much more convenient to have a single grammar element, Class, for both.

@bernardnormier bernardnormier added enhancement New feature or request proposal labels Jul 30, 2024
@bernardnormier bernardnormier added this to the Future milestone Jul 30, 2024
@InsertCreativityHere
Copy link
Member

InsertCreativityHere commented Sep 10, 2024

After thinking it over, I'm no longer convinced this would actually simplify things.

Remember that slicec-cs is transient. The end goal is to rewrite it in C#. So, how we represent it in Rust is irrelevant to C#.
slicec-cs is free to have a single type for both Exception and Class, independent of how the compiler stores them.

So, put slicec-cs aside. And the C# mapping. All there is to consider is slicec.
And within slicec merging these two things together actually causes more complication than it solves.


The implied implementation for this is to delete the Exception type, and add a bool: is_exception field to Class.
But, there are too many places where the distinction between classes and exceptions matters, and having to check this bool (instead of just relying on the type system) adds up.

Most importantly, Class is a type, whereas Exception is not.
Actually, Exception is very limited in Slice. They can only appear in throws clauses, or as a base exception.

So, while they do share almost all their fields, they have very different behavior/use-cases. To the point where there is actually, literally 0 overlap in functionality; ie, anywhere where you can use a class, you cannot use an exception instead, and vice-versa.


Also I just think it's cleaner and more readable to have separate types.
Otherwise with a single type, I think it would be deceptively easy to add some logic in the future, which forgets about this caveat (where some classes are exceptions) which breaks something in a subtle (and likely under-tested) way.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request proposal
Projects
None yet
Development

No branches or pull requests

2 participants