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

Generalize improvement of initialization type frames #642

Merged
merged 9 commits into from
Nov 28, 2023

Conversation

flo2702
Copy link
Collaborator

@flo2702 flo2702 commented Nov 27, 2023

This fixes #640 and fixes #641 by generalizing the fix in #621 . While that fix worked by overriding the error reporting for (common) assignments and method calls in the Initialization Visitor, this fix is general and assigns the correct type to the receiver in the first place in the Initialization ATF, so that overriding specific error-reporting is now unnecessary.

This will obviously conflict with #623 , which uses the overridden error-reporting functions to modify the error messages, so we need to consider how to handle this. The simplest way is to have #623 override this branch, which has the slight disadvantage that some of the code between the Visitor and ATF will then be doubled. Also #623 has the same problem as #621 in that it only applies to (common) assignments and method calls, not, e.g., to the example from #640.

@flo2702 flo2702 requested a review from wmdietl November 27, 2023 14:54
Copy link
Member

@wmdietl wmdietl left a comment

Choose a reason for hiding this comment

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

@flo2702 Thanks a lot for the quick fix of these two issues!
This new way looks much nicer.

I was thinking whether to remove reportCommonAssignmentError, because now there is no longer any override and only a single call. But I think the symmetry to reportMethodInvocabilityError is nice and that method is still overridden once.

I expanded the tests a bit and all looks good.
The change to Issue5174.out is just new store numbers this time. A bit mysterious, but fine.

Thanks!

@wmdietl wmdietl enabled auto-merge (squash) November 28, 2023 03:09
@wmdietl wmdietl merged commit d7f9dab into eisop:master Nov 28, 2023
41 of 42 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants