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

Make typechecker compositional #5461

Closed
wants to merge 2 commits into from

Conversation

catamorphism
Copy link
Contributor

r? @nikomatsakis The typechecker previously passed around a boolean return flag to
indicate whether it saw something with type | (that is, something
it knows at compile-time will definitely diverge) and also had some
manual checks for the ty_err pseudo-type that represents a previous
type error. This was because the typing rules implemented by the
typechecker didn't properly propagate | and ty_err. I fixed it.

This also required changing expected error messages in a few tests,
as now we're printing out fewer derived errors -- in fact, at this
point we should print out no derived errors, so report any that
you see (ones that include "[type error]") as bugs.

The typechecker previously passed around a boolean return flag to
indicate whether it saw something with type _|_ (that is, something
it knows at compile-time will definitely diverge) and also had some
manual checks for the `ty_err` pseudo-type that represents a previous
type error. This was because the typing rules implemented by the
typechecker didn't properly propagate _|_ and ty_err. I fixed it.

This also required changing expected error messages in a few tests,
as now we're printing out fewer derived errors -- in fact, at this
point we should print out no derived errors, so report any that
you see (ones that include "[type error]") as bugs.
@nikomatsakis
Copy link
Contributor

Tim---this basically looks great! I didn't read it through in utmost detail, more skimmed over. This seems prone to fast bitrot so I thought it better to give r+ now and we can refine later. One improvement I would like is if you would modify the comment in typeck/check/mod.rs (or create one on check_expr) that explains the invariants here. For example, is it an invariant that if the thing has type bottom, it will hvae ty_bot (and not some type which has bot embedded?) Similarly for error? I'm not sure that is the right invariant, actually, since it seems like more work to maintain than is worth the trouble, but that's debatable and in any case I'd like to know.

@catamorphism
Copy link
Contributor Author

@nikomatsakis No, that's not the invariant -- I wanted it to be, but wasn't able to implement it because if I tried to change ty::mk_t_with_id to return ty_bot when given, say, ~[ty_bot], I got memory corruption. So instead, "has_bot" and "has_err" are just type flags and the predicates type_is_bot and type_is_error check them. Thus, observationally, the invariant that if a thing has type bot then its type will be ty_bot is true, assuming the caller always uses the predicates and doesn't match on the type structure directly. I'll add a comment to this effect.

@catamorphism
Copy link
Contributor Author

Added a comment -- @nikomatsakis r? just to make sure I was clear.

bors added a commit that referenced this pull request Mar 21, 2013
r? @nikomatsakis The typechecker previously passed around a boolean return flag to
indicate whether it saw something with type _|_ (that is, something
it knows at compile-time will definitely diverge) and also had some
manual checks for the `ty_err` pseudo-type that represents a previous
type error. This was because the typing rules implemented by the
typechecker didn't properly propagate _|_ and ty_err. I fixed it.

This also required changing expected error messages in a few tests,
as now we're printing out fewer derived errors -- in fact, at this
point we should print out no derived errors, so report any that
you see (ones that include "[type error]") as bugs.
@bors bors closed this Mar 21, 2013
oli-obk pushed a commit to oli-obk/rust that referenced this pull request May 2, 2020
…st, r=matthiaskrgr

Temporarily disable rustfmt integration test

Running rustfmt from master is currently broken and [fails our bors build](https://github.com/rust-lang/rust-clippy/runs/582066368#step:10:19):
rust-lang#71077

changelog: none
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.

3 participants