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

Added support for partial type hints (Foo<_>). #12764

Merged
merged 1 commit into from
Mar 14, 2014

Conversation

Kimundi
Copy link
Member

@Kimundi Kimundi commented Mar 8, 2014

Summary

This patch introduces the _ token into the type grammar, with the meaning "infer this type".
With this change, the following two lines become equivalent:

let x = foo();
let x: _ = foo();

But due to its composability, it enables partial type hints like this:

let x: Bar<_> = baz();

Using it on the item level is explicitly forbidden, as the Rust language does not enable global type inference by design.

This implements the feature requested in #9508.

Things requiring clarification

  • The change to enable it is very small, but I have only limited understanding of the related code, so the approach here might be wrong.
    • In particular, while this patch works, it does so in a way not originally intended according to the code comments.
  • This probably needs more tests, or rather feedback for which tests are still missing.
  • I'm unsure how this interacts with lifetime parameters, and whether it is correct in regard to them.
  • Partial type hints on the right side of as like &foo as *_ work in both a normal function contexts and in constexprs like static foo: *int = &'static 123 as *_. The question is whether this should be allowed in general.

Todo for this PR

  • The manual and tutorial still needs updating.

Bugs I'm unsure how to fix

  • Requesting inference for the top level of the right hand side of a as fails to infer correctly, even if all possible hints are given:

    .../type_hole_1.rs:35:18: 35:22 error: the type of this value must be known in this context
    .../type_hole_1.rs:35     let a: int = 1u32 as _;
                                           ^~~~
    

@huonw
Copy link
Member

huonw commented Mar 8, 2014

cc @nikomatsakis

Also there's a comment on the TyInfer variant declaration in syntax::ast that needs to be updated with this change (specifically, TyInfer is now allowed in places other than the top level of a type).

@flaper87
Copy link
Contributor

flaper87 commented Mar 8, 2014

I agree it should be discussed further. Quite useful thing to have and a rather small change required.

I'd like to see the changes in the docs as part of this PR or at least an issue created to keep track of it.


fn fn_test8(_f: fn() -> _) { }
//~^ ERROR type inference is not allowed on items
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Just wondering, is type hint allowed on method?

@ktt3ja
Copy link
Contributor

ktt3ja commented Mar 9, 2014

Maybe adding a compile-fail test for the case when type hint is completely wrong would be good, too.

@pcwalton
Copy link
Contributor

This has been agreed upon.

@@ -132,7 +132,8 @@ impl AstConv for CrateCtxt {
}

fn ty_infer(&self, span: Span) -> ty::t {
self.tcx.sess.span_bug(span, "found `ty_infer` in unexpected place");
self.tcx.sess.span_err(span, "type inference is not allowed on items");
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you make this more specific? Not everyone will know "type inference" means _.

@erickt
Copy link
Contributor

erickt commented Mar 12, 2014

cc-ing #10448, which talks about adding type hints for top-level items.

@huonw
Copy link
Member

huonw commented Mar 13, 2014

GHC got a very proposal recently: http://www.haskell.org/pipermail/ghc-devs/2014-March/004239.html

@@ -1273,6 +1273,10 @@ impl Parser {
bounds
} = self.parse_path(LifetimeAndTypesAndBounds);
TyPath(path, bounds, ast::DUMMY_NODE_ID)
} else if self.token == token::UNDERSCORE {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use self.eat(&token::UNDERSCORE) and remove the self.bump

@Kimundi
Copy link
Member Author

Kimundi commented Mar 13, 2014

That GHC proposal is very interesting, especially the generalizations and extensions listed there. Though I don't think we'll need anything more fancy than what is implemented here for a while.

@nikomatsakis
Copy link
Contributor

Note: although I did r+, it is... mildly unclear to me if _ should be permitted in as expressions at all. It seems strange to cast without saying what you're casting to. But I guess it's useful to be able to partially say what you're casting to. Well, we can tweak this detail as we feel.

@Kimundi
Copy link
Member Author

Kimundi commented Mar 14, 2014

@nikomatsakis Hm, I personally did not see why let x = y as Z should not be seen as a build in function let x = cast::<_, Z>(y), so I expected it to work with this change.

@pnkfelix That indeed looks like a legitimate failure. I had trouble with my build environment the last few days that prevented me from simply running make check. (unrelated debug info test failures that I couldn't probably analysis till now)

I though I had run all relevant test suites manually, but it seems I forgot about the pretty printer, sorry. Now that I see the error, its obvious to me that it would happen. :)

@nikomatsakis
Copy link
Contributor

@Kimundi Good point. The reason that the type check doesn't work is because we check immediately that the as conversion is legal. We could just say that the type of <expr> as T is T and then go back and check later that the type conversion is legal once types are known. I kind of like that. Clearly a separable task though.

@eddyb
Copy link
Member

eddyb commented Mar 14, 2014

@nikomatsakis I did mention the same approach on IRC whenever someone wanted to fix as, but I guess they never got that far. It would fix some nasty cases in closures, where you need to annotate everything.

inference in a type with `_` ). This enables partial type inference.
bors added a commit that referenced this pull request Mar 14, 2014
# Summary

This patch introduces the `_` token into the type grammar, with the meaning "infer this type".
With this change, the following two lines become equivalent:
```
let x = foo();
let x: _ = foo();
```
But due to its composability, it enables partial type hints like this:
```
let x: Bar<_> = baz();
```

Using it on the item level is explicitly forbidden, as the Rust language does not enable global type inference by design.

This implements the feature requested in #9508.

# Things requiring clarification

- The change to enable it is very small, but I have only limited understanding of the related code, so the approach here might be wrong.
  - In particular, while this patch works, it does so in a way not originally intended according to the code comments.
- This probably needs more tests, or rather feedback for which tests are still missing.
- I'm unsure how this interacts with lifetime parameters, and whether it is correct in regard to them.
- Partial type hints on the right side of `as` like `&foo as *_` work in both a normal function contexts and in constexprs like `static foo: *int = &'static 123 as *_`. The question is whether this should be allowed in general.

# Todo for this PR

- The manual and tutorial still needs updating.

# Bugs I'm unsure how to fix

- Requesting inference for the top level of the right hand side of a `as` fails to infer correctly, even if all possible hints are given:

  ```
.../type_hole_1.rs:35:18: 35:22 error: the type of this value must be known in this context
.../type_hole_1.rs:35     let a: int = 1u32 as _;
                                           ^~~~
```
@bors bors closed this Mar 14, 2014
@bors bors merged commit eb69eb3 into rust-lang:master Mar 14, 2014
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 25, 2022
Remove deprecate action

https://old.reddit.com/r/rust/comments/vyx4oj/actionsrs_organization_became_unmaintained/

Looking at this holistically, I don't fully understand *why* we need an
action here? Seems like we can just use rustup? nowadays github runners
come with rustup pre-installed.
flip1995 pushed a commit to flip1995/rust that referenced this pull request May 17, 2024
`significant_drop_in_scrutinee`: Fix false positives due to false drops of place expressions

Place expressions do not really create temporaries, so they will not create significant drops. For example, the following code snippet is quite good (rust-lang#8963):
```rust
fn main() {
    let x = std::sync::Mutex::new(vec![1, 2, 3]);
    let x_guard = x.lock().unwrap();
    match x_guard[0] {
        1 => println!("1!"),
        x => println!("{x}"),
    }
    drop(x_guard); // Some "usage"
}
```

Also, the previous logic thinks that references like `&MutexGuard<_>`/`Ref<'_, MutexGuard<'_, _>>` have significant drops, which is simply not true, so it is fixed together in this PR.

Fixes rust-lang/rust-clippy#8963
Fixes rust-lang/rust-clippy#9072

changelog: [`significant_drop_in_scrutinee`]: Fix false positives due to false drops of place expressions.

r? `@blyxyas`
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.

9 participants