Add some tests for typechecking error messages#8874
Add some tests for typechecking error messages#8874gasche merged 5 commits intoocaml:trunkfrom Lereena:typing-misc-add
Conversation
|
I'm not going to comment on the formatting changes, but I will say that getting coverage reports is very cool! Do you plan to publish the tool somewhere? PS: I find it funny that your new tests mostly don't intersect with the ones added by #8516 :) |
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
This looks very nice, thanks! See a minor comment inline.
Could you add a Changes entry for the PR?
typing/includeclass.ml
Outdated
| fprintf ppf "The public method %s cannot become private" lab | ||
| | CM_Virtual_method lab -> | ||
| fprintf ppf "@[The virtual method %s cannot become concrete" lab | ||
| fprintf ppf "The virtual method %s cannot become concrete" lab |
There was a problem hiding this comment.
This is clearly a good fix, but I think that it would be even better to pack every of those messages (those that are just fprintf ppf ..., not the other ones) in a box (@[...@]), rather than removing the box completely. The balancing bug appears to have been introduced in b96208b, and it seems that the previous code did have boxes on all messages.
There was a problem hiding this comment.
Indeed, now I see that initially there were boxes here, so probably, the better solution would be to return them back.
@trefis About the tool, I modified bisect_ppx to instrument the compiler, but now a build process is not very smooth and there are doubts about maintainability of this solution. So it will take some more time to put everything in order and then present to the public a complete way of generating reports for the compiler. |
gasche
left a comment
gasche
left a comment
There was a problem hiding this comment.
This is good to go if/when the CI agrees. (I may not be around for merging, so anyone please feel free.)
|
@Lereena If changes are needed to Bisect_ppx, we'd be happy to merge them to make using it on the compiler easier. |
|
@aantron , quite a few of the changes were about removing bell and whistles from the ppx source code to make it easier to build with the in-tree compiler and do not really make sense outside of this specific context; but we may try to upstream some of the less invasive patchsets. |
5 participants
In the process of creating a tests coverage report for the compiler code using
bisect_ppx, I generated a report specifically for the typechecking subsystem (you can see it here). Using this coverage report I found some gaps that could be easily fixed with few tests, so I added a small one for invalid type variable names and, more importantly, I added various tests covering all error messages related to inclusion checks for the class language.Several unpaired opens of pretty-printing boxes in the error messages have also been removed to prevent printing errors.
I also used those tests to check that the coverage report works fine: after adding tests, the expected parts of the code were highlighted (before, after).
The bisect_ppx integration work is done under the guidance of @Octachron and @shindere.