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

WIP: Fix incorrect backtraces for errors in certain boolean expressions (DO NOT MERGE, but please discuss) #3067

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

fingolfin
Copy link
Member

This is a bit like playing an endless game of whack-a-mole, or putting lipstick on a pig, of course -- the proper fix would be to rewrite the code tracking subexpressions inside statements, but that's a bigger change, so let's fix #3044 the simple way.

The first commit adds a couple of tests that reproduce #3044 and various more or less related issues.

The second commit fixes the problem, and updates the tests. By looking at just the diff for the second commit, you can thus easily see how those cases changed.

But after implementing this, I realized that there are many more similar issues, requiring lots of manual changes... or a sledgehammer approach to try to get rid of all of them at once. The third and fourth commit represent two different approaches to doing that, with different pros and cons. Note that the fourth commit is a hack, and probably introduces too much over head anyway, which is why I did not bother to clean it up; but it is good to see it "for comparison". Also, in fact, all of these changes may cause performance regressions, and should be benchmarked.

As discussed recently in St Andrews, even if one ignores all practical obstacles, part of the issue here is that it is not always clear what's "best" to print. I.e., even if we ignore the question "how to implement the desired behaviour?", it is not even clear what the desired behaviour is.

@fingolfin fingolfin added kind: bug Issues describing general bugs, and PRs fixing them kind: request for comments regression A bug that only occurs in the branch, not in a release do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes kind: discussion discussions, questions, requests for comments, and so on labels Nov 28, 2018
@ChrisJefferson
Copy link
Contributor

If the overhead of the 4th commit isn't too great, it would be nice to handle this in one place -- and if we have to effectively put calls around almost every expression anyway, it might not be much slower

@fingolfin fingolfin force-pushed the mh/track-bool-exprs branch from 99b8b2e to 9adaedb Compare August 5, 2022 11:45
@fingolfin fingolfin force-pushed the mh/track-bool-exprs branch from 9adaedb to 0cf6ce2 Compare August 5, 2022 11:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
do not merge PRs which are not yet ready to be merged (e.g. submitted for discussion, or test results) kind: bug Issues describing general bugs, and PRs fixing them kind: discussion discussions, questions, requests for comments, and so on regression A bug that only occurs in the branch, not in a release release notes: not needed PRs introducing changes that are wholly irrelevant to the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Wrong statement shown in error message
2 participants