-
Notifications
You must be signed in to change notification settings - Fork 119
fix: Redundant type instantiation wrong when return expression
#5605
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
base: master
Are you sure you want to change the base?
Conversation
src/mo_frontend/typing.ml
Outdated
| let closed_codom = Bi_match.is_closed remaining codom in | ||
| (* Closed [codom] implies closed [body_typ]. [body_typ] is closed when it comes from [typ_opt] *) | ||
| let closed_body_typ = closed_codom || Option.is_some typ_opt in | ||
| let env' = if closed_body_typ then env' else { env' with rets = None } in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if env'.rets = None, that means the body can't return at all which is weird because functions can always return. There's only a few places where we disallow returns (e.g. object/class constructors) so maybe we should discuss.
Are you ever checking the body of a function with env'.rets = None? If not, probably ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I think you are at line 2989
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good point. It's better to distinguish between None and Some here for rets
It should be more like type rets = NotAllowed | RetBimatch | Ret typ.
And a custom error when trying to check when RetBimatch saying that cannot infer type of return etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Silly thought, and I haven't thought it through, but I wonder if one could get away with only updating the notes once, when they contain T.Pre (or a PreLit), and just not apply any further update.
return expression
We need to perform the redundancy check before |
src/mo_frontend/typing.ml
Outdated
| check_exp_strong env t exp1 | ||
| | None -> | ||
| | BimatchRet k -> | ||
| k env exp1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will actually wind up doing infer_exp rather than check_exp_strong (as on 2084). God knows if that would make a difference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing infer is expected, but true I can pass the weak = false here
Continuation in a separate PR here #5615
return expressionreturn expression
Fix the
M0223warning check for redundant type instantiations to avoid false positives.This check is now correctly performed during
not env.prephase like all other warnings.Also: Correctly handles nested expressions, further avoiding false positives.