-
Notifications
You must be signed in to change notification settings - Fork 165
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
Add type check on if-expr #3306
base: master
Are you sure you want to change the base?
Conversation
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.
I think this is good, thank you!! Just waiting on Philip to check it as he knows the typechecker :D
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.
LGTM
dd05fff
to
ec71cb1
Compare
ec71cb1
to
4786b64
Compare
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 is now rebased and looks like there are failing tests can you investigate?
4786b64
to
557f32b
Compare
ok = context->lookup_builtin ("()", &null_ty); | ||
rust_assert (ok); | ||
|
||
coercion_site (expr.get_mappings ().get_hirid (), |
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.
What happens if you use the result of this coercion_site? so it becomes:
infered = coercion_site (...)
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.
Right, that actually makes a lot of sense - sorry I skipped over this. Trying it now
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.
I think this LGTM but i would prefer if we use the result of the coercion_site if that works if not then let us know and we can go with this.
Also we really need to make an issue about making much nicer helpers for looking up those builtin types
Check if an if-expr returns void type or a coercible type like an early return. gcc/rust/ChangeLog: * typecheck/rust-hir-type-check-expr.cc (TypeCheckExpr::visit): Add check on if-expr. gcc/testsuite/ChangeLog: * rust/compile/implicit_returns_err3.rs: Change test to be valid. * rust/compile/torture/if.rs: Likewise. * rust/compile/if-without-else.rs: New test. Signed-off-by: Benjamin Thos <[email protected]>
557f32b
to
35bd75d
Compare
Check if an if-expr return void type.
gcc/rust/ChangeLog:
gcc/testsuite/ChangeLog:
Fixes: #3230