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 #9136 - inconsistent handling of exception return #9137

Conversation

isCzech
Copy link
Contributor

@isCzech isCzech commented Apr 24, 2021

Fixes #9136

@isCzech
Copy link
Contributor Author

isCzech commented Apr 27, 2021

Hi,
Apologies, I changed my mind: the two kinds of return as in

[] on: Error do: [:ex | ex return]

and

[] on: Error do: []

really are two distinct operations (and probably should remain as such). They are equivalent in Pharo thanks to their current implementation. I'm closing the PR.

@isCzech isCzech closed this Apr 27, 2021
@dionisiydk
Copy link
Contributor

Hi @isCzech

I am more convinced with your example that your proposal is correct.
Imagine that #return: is redefined for some exception class. Then the version with "ex return" will works differently than with simple block result. So they are not equivalent with the current implementation

@isCzech
Copy link
Contributor Author

isCzech commented Apr 27, 2021

Thanks @dionisiydk for your feedback, I've done some more testing and realized there will be a problem with Exception>> #outer - I'll come up with a test showing it later - so the "fix" probably wouldn't be as simple as I suggested. Besides, as Stephane writes in Deep into Pharo (2013, p. 278), ANSI is not clear regarding the potential difference - or equivalence - between the two. What happened is I'd experimented with a modification of #return and realized the implicit return (or abandon as it's referred to in Deep Into Pharo) will become different from #return :)

@isCzech
Copy link
Contributor Author

isCzech commented Apr 28, 2021

Hi @dionisiydk , you were right, my bad - I got confused and the suggested modification works well with #outer. I'm reopening the PR for your consideration :) Sorry for the confusion. Thanks

@Ducasse
Copy link
Member

Ducasse commented Jun 14, 2021

I suggest that we keep this one for Pharo 10 because we really want to make sure that we do not have changes that can shake to o much the system. Pharo 90 should have been out in April. :(

@guillep
Copy link
Member

guillep commented Nov 24, 2021

Hi all, it's been really difficult to review the batch of issues (#8567 , #8845, #9137, #8993, #8509).
We have spent in the last internal sprints several hours taking a look at them, but it is a reality that those changes are difficult to review, some like #8567 are very big, their effects in the system are difficult to foresee independently and even worse in combination.

@tesonep is correctly (!!) proposing to my ear that we make a single PR joining all of them to reduce the merge noise, let the CI test the combination of all issues. I then propose that we eagerly merge this (meta) PR and be ready to quickly apply hot-fixes as soon as possible or revert it in case of fire.

Thoughts? @dionisiydk @Ducasse @isCzech @estebanlm

@guillep
Copy link
Member

guillep commented Nov 24, 2021

Superseeded by #10429

@guillep guillep closed this Nov 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent explicit and implicit exception return
5 participants