Skip to content

Comments

Allow Cogit>>#mnuMethodOrNilFor: to return non-CompiledMethod oops#936

Merged
guillep merged 2 commits intopharo-project:pharo-12from
iglosiggio:fix-run-with-in-for-dnu
Mar 31, 2025
Merged

Allow Cogit>>#mnuMethodOrNilFor: to return non-CompiledMethod oops#936
guillep merged 2 commits intopharo-project:pharo-12from
iglosiggio:fix-run-with-in-for-dnu

Conversation

@iglosiggio
Copy link
Collaborator

This should fix #935. The origin of this issue appears to be from a partially applied fix (previously we tried to compile method wrappers, then we just skipped them. Now we don't try to compile them but we properly return them as valid "methods" (given that we have the #run:with:in semantics for method invocation of regular objects).

See #578 for more info.

@iglosiggio
Copy link
Collaborator Author

iglosiggio commented Mar 25, 2025

Looking at the previous change (#578) this PR may break some tests. I'm not convinced those tests are correct though.

@guillep
Copy link
Member

guillep commented Mar 27, 2025

@iglosiggio
Copy link
Collaborator Author

indeed there is one broken test (failing in four different configs):

Exactly, I think the new behavior is correct. The test is trying to check that the method was not compiled, if it was not a method. There's no way to check that a non-method was not compiled because only methods should be compiled. What the test tries to assert is not expressable as a valid idea.

Maybe it is enough to assert that the lookup result is not a compiled method and that the counter for compiled methods was kept at a constant value before/after the lookup. I'm not sure tbh.

@iglosiggio
Copy link
Collaborator Author

Should be fixed now :)

@guillep guillep force-pushed the fix-run-with-in-for-dnu branch from 7905638 to 7514d2a Compare March 28, 2025 15:11
Copy link
Member

@guillep guillep left a comment

Choose a reason for hiding this comment

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

Thanks @iglosiggio mega for the great energy :)

@guillep guillep merged commit 8503671 into pharo-project:pharo-12 Mar 31, 2025
1 check failed
@iglosiggio iglosiggio deleted the fix-run-with-in-for-dnu branch April 28, 2025 02:16
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