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

Guide: Some Nitpicking #17191

Closed
mitsuhiko opened this issue Sep 12, 2014 · 9 comments
Closed

Guide: Some Nitpicking #17191

mitsuhiko opened this issue Sep 12, 2014 · 9 comments

Comments

@mitsuhiko
Copy link
Contributor

  • The ok() documentation in the read_line example is not just inaccurate, it's misleading and wrong. .ok() does not abort the program, it returns an option. Maybe it needs to have an option section before to explain that type.
  • the random number code uses % to make random numbers. This is a bad idea and generally guides are something people copy out of, so I would propose not bringing an antipattern into the mix.
@MatejLach
Copy link
Contributor

  • While I agree that it could've been worded better, Option<T> is explained in quite a bit of detail in this section and of course here.
    Perhaps it could be added that ok() returns an Option, altrough I feel that the Guide makes reasonable compromises in that it cannot cover everything at once.
    If one reads the whole thing, it becomes pretty clear what ok() does and if it's still unclear, one can look up the API docs (with examples for even better clarity).

Since the details of Option are explained right in the next chapter, I think it's fine as it is, but prehaps one could write something like:

"Rust provides a method on these IoResults called ok(), which does the same thing as our match statement, but assuming that we have a valid value. If we don't, it will return an Option, which we will explore in the next chapter, (for now, we can say that it will terminate our program)" or something like that.
I don't think any massive refactoring is needed, but I wonder what @steveklabnik thinks about this.

@huonw
Copy link
Member

huonw commented Sep 12, 2014

the random number code uses % to make random numbers. This is a bad idea and generally guides are something people copy out of, so I would propose not bringing an antipattern into the mix.

@steveklabnik People keep noticing and commenting on this, so I think we should at least have a more obvious note pointing people to gen_range as the correct solution, if not updating the code itself.

@huonw huonw added the A-docs label Sep 12, 2014
@steveklabnik
Copy link
Member

The abort thing is just a straight-up error on my part, it should say that expect is what does the termination, you're right.

Is there a way we can have the range function without a trait? That's the blocking issue here.

@mitsuhiko
Copy link
Contributor Author

+1 on a range function. The fact that the obvious way to do this is too hard for beginner docs indicates that an API adjustment might be a good idea.

@steveklabnik
Copy link
Member

/cc @aturon , what do you think about removing the need for this trait?

@aturon
Copy link
Member

aturon commented Oct 3, 2014

@steveklabnik We could potentially do it by adding a default type parameter. Would that suffice? Or do you need to show the signature in full (in which case this would make it even more complex)?

@steveklabnik
Copy link
Member

A default parameter would be fine. I don't actually need to show the type signature.

@steveklabnik
Copy link
Member

@huonw is going to work on stabilizing rand, so hopefully we can just get something in here that doesn't require the trait import.

@steveklabnik
Copy link
Member

I'm giving this a close, finally, as it's not even sure that rand is staying in the introduction at this point.

lnicola pushed a commit to lnicola/rust that referenced this issue Aug 13, 2024
fix: Insert a generic arg for `impl Trait` when lowering generic args

Fixes rust-lang#17191

We are not inserting a generic arg when lowering generics like
```rust
fn foo<T: B<impl A>(..) { ... }
```
but when we are lowering predicates we do;

https://github.com/rust-lang/rust-analyzer/blob/aa00ddcf654a35ba0eafe17247cf189958d33182/crates/hir-ty/src/lower.rs#L1697-L1718
https://github.com/rust-lang/rust-analyzer/blob/aa00ddcf654a35ba0eafe17247cf189958d33182/crates/hir-ty/src/lower.rs#L310

and this mismatch causes index out of bound panic while substituting the predicates
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

No branches or pull requests

5 participants