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

Partially implement type ascription #30184

Merged
merged 3 commits into from
Dec 19, 2015
Merged

Partially implement type ascription #30184

merged 3 commits into from
Dec 19, 2015

Conversation

petrochenkov
Copy link
Contributor

This PR is a rebase of the original PR by @eddyb #21836 with some unrebasable parts manually reapplied, feature gate added + type equality restriction added as described below.

This implementation is partial because the type equality restriction is applied to all type ascription expressions and not only those in lvalue contexts. Thus, all difficulties with detection of these contexts and translation of coercions having effect in runtime are avoided.
So, you can't write things with coercions like let slice = &[1, 2, 3]: &[u8];. It obviously makes type ascription less useful than it should be, but it's still much more useful than not having type ascription at all.
In particular, things like let v = something.iter().collect(): Vec<_>; and let u = t.into(): U; work as expected and I'm pretty happy with these improvements alone.

Part of #23416

@petrochenkov
Copy link
Contributor Author

Hm, @rust-highfive failed to assign a reviewer.

r? @eddyb

@eddyb
Copy link
Member

eddyb commented Dec 3, 2015

I wrote the original version of this code, so I'd rather have someone else look over it.
r? @nikomatsakis

@rust-highfive rust-highfive assigned nikomatsakis and unassigned eddyb Dec 3, 2015
@@ -2671,6 +2671,14 @@ fn check_lit<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
}
}

fn check_expr_eq_type<'a, 'tcx>(fcx: &FnCtxt<'a, 'tcx>,
expr: &'tcx hir::Expr,
Copy link
Contributor

Choose a reason for hiding this comment

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

wrong indentation

@bors
Copy link
Contributor

bors commented Dec 4, 2015

☔ The latest upstream changes (presumably #29850) made this pull request unmergeable. Please resolve the merge conflicts.

@petrochenkov
Copy link
Contributor Author

Rebased.

@bors
Copy link
Contributor

bors commented Dec 16, 2015

☔ The latest upstream changes (presumably #30206) made this pull request unmergeable. Please resolve the merge conflicts.

eddyb and others added 2 commits December 16, 2015 17:12
@petrochenkov
Copy link
Contributor Author

Rebased.

@nikomatsakis
Copy link
Contributor

Sorry for the delay! I'll get to this soon.

@@ -320,6 +320,8 @@ impl<'tcx> Mirror<'tcx> for &'tcx hir::Expr {
name: Field::new(index.node as usize) },
hir::ExprCast(ref source, _) =>
ExprKind::Cast { source: source.to_ref() },
hir::ExprType(ref source, _) =>
return source.make_mirror(cx),
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't seem right. I think we probably want to convert this to a ExprKind::Cast.

Copy link
Contributor

Choose a reason for hiding this comment

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

/me reconsiders. Actually, let me read a bit more and get back to this point!

@nikomatsakis
Copy link
Contributor

@petrochenkov I didn't notice at first that you skipped over the coercion aspect. Makes sense for a first pass, this certainly simplifies things. The code looks great. The only thing I would want to add are some parsing tests that test the relative precedence of : with other operators. Can you do that?

@nikomatsakis
Copy link
Contributor

Some examples tests for precedence:

  • &[1, 2, 3]: &[u8; 3] => (&[1,2,3]): &[u8: 3]
  • x:u8+y => (22:u8)+42
  • x+y:u8 => x+(y:u8)

The only tricky part is how to write such at est. I envisioned testing this by setting up some scenarios that will fail to type-check if the precedence is not correct? I'm not sure if there is a better way.

@petrochenkov
Copy link
Contributor Author

Sorry for the delay! I'll get to this soon.

Thanks! In fact my free time is coming to an end rapidly, so I have to wrap up my work on rustc ASAP (at least for now).
I'll add tests for operator precedence.

@nikomatsakis
Copy link
Contributor

@petrochenkov

Thanks! In fact my free time is coming to an end rapidly, so I have to wrap up my work on rustc ASAP (at least for now).

:( Let me know if there is work you think you will not have time for! I'd be happy to land this PR (or privacy) and then do the followup work myself.

@petrochenkov
Copy link
Contributor Author

Updated with the operator precedence tests.

@nikomatsakis
Copy link
Contributor

@bors r+

Another nice PR. Thanks @petrochenkov!

@bors
Copy link
Contributor

bors commented Dec 19, 2015

📌 Commit 95fdaf2 has been approved by nikomatsakis

@bors
Copy link
Contributor

bors commented Dec 19, 2015

⌛ Testing commit 95fdaf2 with merge 440ef8b...

bors added a commit that referenced this pull request Dec 19, 2015
This PR is a rebase of the original PR by @eddyb #21836 with some unrebasable parts manually reapplied, feature gate added + type equality restriction added as described below.

This implementation is partial because the type equality restriction is applied to all type ascription expressions and not only those in lvalue contexts. Thus, all difficulties with detection of these contexts and translation of coercions having effect in runtime are avoided.
So, you can't write things with coercions like `let slice = &[1, 2, 3]: &[u8];`. It obviously makes type ascription less useful than it should be, but it's still much more useful than not having type ascription at all.
In particular, things like `let v = something.iter().collect(): Vec<_>;` and `let u = t.into(): U;` work as expected and I'm pretty happy with these improvements alone.

Part of #23416
@bors bors merged commit 95fdaf2 into rust-lang:master Dec 19, 2015
@petrochenkov
Copy link
Contributor Author

This should probably be marked with relnotes

@eddyb eddyb added the relnotes Marks issues that should be documented in the release notes of the next release. label Dec 19, 2015
@brson
Copy link
Contributor

brson commented Jan 8, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants