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

Fix "analysis failed but no diagnostics were emitted" panics #534

Merged
merged 1 commit into from
Sep 6, 2021

Conversation

sbillig
Copy link
Collaborator

@sbillig sbillig commented Sep 4, 2021

What was wrong?

closes #532 - Here we weren't emitting an error message if the String type constructor was ill-formed. Eg String<x>("hi") would be rejected without an error message.

closes #530 - Here we were panicking because the fn body diagnostic vec was empty, even though the analysis failed. We shouldn't panic in this case, because an undefined type diagnostic was produced elsewhere (during the analysis of the function signature).

These cases are just symptoms of a problem in the analyzer; we require that a friendly diagnostic be emitted before returning FatalError or TypeError, but can't effectively ensure that that's happening.

How was it fixed?

FatalError and TypeError now can't be created unless you have a voucher proving that you've emitted a diagnostic. You can receive such a voucher by calling an AnalyzerContext error function. For example:

Err(TypeError::new(scope.error("something is wrong", span, "this")))

I noticed some other error stuff that needs to be improved, and added TODO comments. I'll fix those in a follow-up PR.

To-Do

@codecov-commenter
Copy link

Codecov Report

Merging #534 (31491b1) into master (e256045) will increase coverage by 0.04%.
The diff coverage is 88.88%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #534      +/-   ##
==========================================
+ Coverage   87.68%   87.73%   +0.04%     
==========================================
  Files          87       87              
  Lines        5670     5674       +4     
==========================================
+ Hits         4972     4978       +6     
+ Misses        698      696       -2     
Impacted Files Coverage Δ
crates/analyzer/src/db/queries/functions.rs 92.04% <66.66%> (+0.09%) ⬆️
crates/analyzer/src/traversal/expressions.rs 91.11% <100.00%> (+0.56%) ⬆️
crates/analyzer/src/traversal/types.rs 94.59% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e256045...31491b1. Read the comment docs.

@sbillig sbillig marked this pull request as draft September 5, 2021 01:41
@sbillig sbillig force-pushed the fn-body-diagnostic-panic branch from 31491b1 to af8e7df Compare September 5, 2021 05:51
@sbillig sbillig marked this pull request as ready for review September 5, 2021 06:12
@sbillig sbillig requested a review from cburgdorf September 5, 2021 06:12
@sbillig sbillig force-pushed the fn-body-diagnostic-panic branch from af8e7df to 996a784 Compare September 5, 2021 17:12
Copy link
Collaborator

@cburgdorf cburgdorf left a comment

Choose a reason for hiding this comment

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

Great hardening of the error system.

/// a `TypeError`. So that the rust compiler can help us enforce this rule, a `TypeError`
/// cannot be constructed without providing a [`DiagnosticVoucher`]. A voucher can be obtained
/// by calling an error function on an [`AnalyzerContext`](crate::context::AnalyzerContext).
/// Please don't try to work around this restriction.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice pattern :)

crates/analyzer/src/traversal/expressions.rs Show resolved Hide resolved
crates/analyzer/src/traversal/expressions.rs Show resolved Hide resolved
@sbillig sbillig merged commit f1327bb into ethereum:master Sep 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants