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

Errors in Analysis #136

Open
bjchambers opened this issue Nov 2, 2021 · 4 comments
Open

Errors in Analysis #136

bjchambers opened this issue Nov 2, 2021 · 4 comments

Comments

@bjchambers
Copy link
Contributor

There are a variety of cases where an analysis might fail -- for instance, if it is trying to put type information it may fail to determine the result type when a new (invalid) node is created.

Some of these can be handled by type-checking before creating the egraph node(s). But, it seems like it could be helpful to allow the Analysis to define an Error type and have the methods return Result<..., Self::Error>. Existing Analysis implementations could use an infallible Error, while analyses that wish to report errors could do so.

This could also make it easier (in some cases) to debug errors using egraph -- if merge is called and produces an error it could indicate why things were being merged, etc. which would be easier to debug than a panic.

@mwillsey
Copy link
Member

mwillsey commented Nov 8, 2021

I agree this could be nice to have. Although I'm unsure about how a user would ever handle these errors. And if the user can't ever handle them, then it seems like just panicking is preferable.

Do you have a use case for fallible union? Or rewrites? How would you recover from that?

@bjchambers
Copy link
Contributor Author

Some uses:

  1. It's used as part of a query compiler for some service, in which case it would be better to return an Err(...) and return an Internal Server Error.
  2. There is additional information that egg could add to such failures. Right now, that all has to be manually added to the panic. For instance, if the merge returns an error, egg could choose to panic, but include information such as what was being merged, why it was being merged, which simplification rule directly lead to it, etc.
  3. You could also imagine cases where there are two sets of simplification rules -- one containing more aggressive or in-development rules compared to the other. If that set produced an error, you may want to instead try the other set. This could possibly also be used within egg to actually skip rules that lead to errors.

@mwillsey
Copy link
Member

Ok, I am convinced of the use case, but this posed to be quite a viral change. Now basically everything can fail: adding e-nodes, unioning things, running rewrites, rebuilding. I'm not sure how much it makes sense to bubble these things up. For example, if you run a rewrite and some union fails at some point, do you keep the stuff the rewrite has done so far? Seems tricky.

@bjchambers
Copy link
Contributor Author

One related thing that may be really helpful (happy to open a separate request) would be including the name of the rule being applied when merge / make/ etc. are being applied (even if it was sometimes None). Right now, my analysis sometimes panics in cases that shouldn't arise (due to mis-written rewrite rules), and it is tricky to debug since I need to manually use a debugger to get the index of the rule name that is being applied, and then look that up in the symbol table.

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