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 long bug in error regions #1049

Merged
merged 3 commits into from
Sep 20, 2023
Merged

Conversation

johnynek
Copy link
Owner

I wanted to merely improve error reporting by showing more than one type error when it was possible (#1044 )

Along the way, I found a long standing bug with locations incorrectly being set to be their outermost scope. I had noticed this before, but hadn't run it down. I had assumed it was part of typechecking and just due to inference being a bit non-specific when unifying across many types.

But in my example, I wound up printing out the Expr and seeing that the Declarations and Regions were set wrong.

The bug turned out to be the .as from Functor. The Expr traverse instance is doing what you say, but .as isn't what we meant. We wanted to only replace the outermost tag, not every tag transitively.

I have removed that code since we were only using it for this one bug. :)

This also should do more parallel checking of errors: if two adjacent lets don't depend on each other, we check in parallel.

We can do a bit better by only skipping lets that depend on a previous failed let, but I will punt that to a later PR.

@codecov-commenter
Copy link

codecov-commenter commented Sep 19, 2023

Codecov Report

Patch coverage: 96.63% and project coverage change: +0.20% 🎉

Comparison is base (423a144) 91.75% compared to head (66405c8) 91.95%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1049      +/-   ##
==========================================
+ Coverage   91.75%   91.95%   +0.20%     
==========================================
  Files          92       93       +1     
  Lines        9549     9626      +77     
  Branches     2255     2258       +3     
==========================================
+ Hits         8762     8852      +90     
+ Misses        787      774      -13     
Files Changed Coverage Δ
...re/src/main/scala/org/bykn/bosatsu/TypedExpr.scala 90.88% <ø> (+1.56%) ⬆️
core/src/main/scala/org/bykn/bosatsu/Expr.scala 89.52% <90.32%> (+1.68%) ⬆️
.../src/main/scala/org/bykn/bosatsu/rankn/Infer.scala 95.70% <98.21%> (+0.06%) ⬆️
...ore/src/main/scala/org/bykn/bosatsu/ListUtil.scala 100.00% <100.00%> (ø)
...src/main/scala/org/bykn/bosatsu/PackageError.scala 69.68% <100.00%> (+0.99%) ⬆️
.../main/scala/org/bykn/bosatsu/SourceConverter.scala 97.59% <100.00%> (+<0.01%) ⬆️

... and 8 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@johnynek johnynek merged commit cef4c90 into master Sep 20, 2023
@johnynek johnynek deleted the oscar/20230918_improve_error_locations branch September 20, 2023 01:17
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

Successfully merging this pull request may close these issues.

2 participants