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

Move HIR into diplomat-tool #596

Open
robertbastian opened this issue Jul 25, 2024 · 3 comments
Open

Move HIR into diplomat-tool #596

robertbastian opened this issue Jul 25, 2024 · 3 comments

Comments

@robertbastian
Copy link
Collaborator

robertbastian commented Jul 25, 2024

HIR is not used by diplomat (the macro), only by diplomat-tool. It's already hidden behind a feature in diplomat-core, so that diplomat doesn't have to build it. Moving it into diplomat-tool would have the advantage of removing all the _ => unreachable!("unknown AST/HIR variant"), that we have because all the HIR types are non_exhaustive.

@Manishearth
Copy link
Contributor

This would just lead to unreachables due to AST non_exhaustives, no?

For a long time I've actually wanted us to enable non_exhaustive_omitted_patterns so that the tool gets the benefit of exhaustive matching. I forgot why we couldn't, might have been a Rust version thing, but we're probably past that barrier now.

The original hope was for third-party backends to use diplomat_core (but not diplomat-tool) to do their thing. We're not quite doing that yet since we're so far happy to steward those backends (and stewarding them leads to faster feedback on the usefulness of various core features), but we may eventually be in such a position as Diplomat matures.

I actually think that getting rid of the non_exhaustive might be the best play here! We don't seem to be modifying the AST/HIR that much these days; most modifications end up in the attribute system (yay), the two big upcoming changes that change HIR are callbacks and option types, where I think a breaking change is warranted anyway. Furthermore we've not shown ourselves shy of making major version bumps of Diplomat, I think that pattern works fine and the non_exhaustive isn't really reducing the number of major bumps we're making.

So I'd say we should get rid of non_exhaustive except perhaps in the attrs code.

@robertbastian
Copy link
Collaborator Author

This would just lead to unreachables due to AST non_exhaustives, no?

Yes, but that's a few unreachables per type during lowering, not dozens of unreachables in each backend during code generation.

@Manishearth
Copy link
Contributor

The original hope was for third-party backends to use diplomat_core (but not diplomat-tool) to do their thing

This is also potentially going to be the long term fate of the java (panama) backend since it's the hardest to run and test and it might be best served by being a separate repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants