Use codespan for analyzer errors #398
Conversation
sbillig
changed the title
Codecov Report
@@ Coverage Diff @@
## master #398 +/- ##
==========================================
- Coverage 86.43% 86.09% -0.35%
==========================================
Files 69 71 +2
Lines 4431 4637 +206
==========================================
+ Hits 3830 3992 +162
- Misses 601 645 +44
Continue to review full report at Codecov.
|
sbillig
force-pushed
the
analyzer-errors
branch
3 times, most recently
from
0a44764 to
78de924
Compare
| @@ -39,6 +66,30 @@ pub fn assign( | |||
| unreachable!() | |||
| } | |||
|
|
|||
| pub fn check_assign_target( | |||
There was a problem hiding this comment.
I now notice that I didn't apply the same check to aug_assign. That can wait for a future pr.
|
@satyamakgec I'd be happy to hold off on this until #385 is merged and deal with the merge conflicts, if your stuff is nearly ready. |
|
Thanks @sbillig, Yeah it is ready for review once it is merged then you will get a lot of conflicts 😬 |
| params: &[(String, FixedSize)], | ||
| ) -> Result<(), SemanticError> { | ||
| for ((label, param_type), arg) in params.iter().zip(args.kind.iter()) { | ||
| let val_attrs = assignable_expr(Rc::clone(&scope), Rc::clone(&context), &arg.kind.value)?; |
There was a problem hiding this comment.
I just noticed that if there are more args than the fn expects, the excess args won't be analyzed. I'm inclined to fix this later.
|
👁️ |
What was wrong?
Analyzer errors weren't fancy enough.
How was it fixed?
This has become a behemoth of a pr, sorry for the hassle.
The context object now contains a vec of diagnostics. Analysis functions will return
Okwith correct type information wherever possible, even if they emit an error diagnostic. In cases where correct type information can't be returned,Err(SemanticError::fatal())is returned. There are still a lot of old errors yet to be converted. In some cases, fancy errors will be followed by an old error, so the user experience will be a little awkward for a while. When they've all been converted to codespan diagnostics, SemanticError/ErrorKind can finally be retired.I had to do a lot of refactoring to give myself enough context to emit pretty error messages. I've been inlining some utility functions and passing around more nodes and spans, and I'm starting to wonder if there's a better way. Eg rustc's "hir" analysis uses various query functions to retrieve such info. Maybe we could structure things similarly as part of the salsa experiment and avoid passing around spans everywhere. Either way I need to take a breather from this for a while; maybe some enlightenment will come later. Y'all are more than welcome to pick away at the remaining old errors, and/or suggest better wording for the errors I did convert. The quality of the error messages is very reflective of my energy level at the time they were written :)
I added a basic check of the left-hand side of assignments; this could be extended to verify that the lhs isn't a read-only attribute of a built-in, for example. The labels of event and struct constructor args are now verified as well. The labels of function args are still ignored (I'd like extend the same checking logic of events/structs to function calls, if the ideas in #397 are agreed upon).
Along the way, I noticed that tuples inside maps weren't being lowered and would thus panic; couldn't resist fixing that, even though it could have been a separate pr.
closes #408
closes #382
To-Do