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

easy fix: lifetimes should contain Name's, not idents #7743

Closed
jbclements opened this issue Jul 12, 2013 · 16 comments · Fixed by #12451
Closed

easy fix: lifetimes should contain Name's, not idents #7743

jbclements opened this issue Jul 12, 2013 · 16 comments · Fixed by #12451
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.

Comments

@jbclements
Copy link
Contributor

this is 20 minutes of work, but I don't think I'm going to do it: lifetimes do not need to be compared hygienically, and they should contain Names, not idents. This will allow them to be safely compared without fear of runtime-fail.

@lilac
Copy link
Contributor

lilac commented Jul 22, 2013

@jbclements I am actively doing this. This fix results in 17 changed files. But one error still remains, which I don't know how to resolve.

src/librustc/front/test.rs:366:28: 366:35 error: mismatched types: expected uint but found syntax::ast::ident (expected uint but found struct syntax::ast::ident)
/Users/james/workspace/rust/rust/src/librustc/front/test.rs:366 pub static TESTS : &'static [self::extra::test::TestDescAndFn] =
^~~~~~~
note: in expansion of quote_item!
/Users/james/workspace/rust/rust/src/librustc/front/test.rs:365:5: 369:6 note: expansion site
error: aborting due to previous error
make: *** [x86_64-apple-darwin/stage0/lib/rustc/x86_64-apple-darwin/lib/librustc.dylib] Error 101

Do you have any hint?

@huonw
Copy link
Member

huonw commented Jul 22, 2013

@lilac it will probably require a snapshot, because it's the quote_item from the old version of the compiler (where lifetimes were still idents) trying to interface with the new libsyntax (where lifetimes are Names).

(It's probably going to require finesse, so talking with us on IRC might be more efficient.)

@lilac
Copy link
Contributor

lilac commented Jul 22, 2013

@huonw I post one message on IRC, but haven't got any response. What kind of finesse do I need?

@huonw
Copy link
Member

huonw commented Jul 22, 2013

@lilac the reason I was vague is because I don't actually know the process that will be required. Approximately, with all the details glossed over:

  1. make the change, but do it such that stage 1 libsyntax still uses idents for lifetime names, but the quote implementations (in stage 1) should use Names. (This will probably require devious hackery.)
  2. take/wait for a snapshot, so that the new stage 0 compiler has the ident -> Name change in it.
  3. Finally remove the stuff about idents.

I have literally no idea if step 1 is even possible. It looks like this issue is more than 20 minutes. :(

(Oh, it's unfortunate that no-one responded :( you could try again during US-daytime hours, I probably won't be there (wrong timezone), but people who know more than will be around. You may need to ask a few times for help; preferably linking to this issue to make it easy for someone to jump in.)

@huonw
Copy link
Member

huonw commented Jul 22, 2013

@lilac Oh, the other solution would be to remove the point where quote_item quotes a lifetime (possibly by writing the raw AST directly at that point); then the change should go smoothly without needing snapshots.

@jbclements
Copy link
Contributor Author

I just want to say: you guys are awesome!

@lilac
Copy link
Contributor

lilac commented Jul 23, 2013

@huonw Thanks, you are really kind and helpful! I am in the AEST timezone, so guess that's the reason why few people were there and replied me. I will try and see if rewriting to raw AST is feasable.

@huonw
Copy link
Member

huonw commented Jul 23, 2013

@lilac Good luck!

(Assuming that's Australian Eastern Standard Time, I am too! I'm in Sydney, fwiw.)

@bstrie
Copy link
Contributor

bstrie commented Jul 23, 2013

IIRC aatch and bjz should be in roughly that timezone as well.

@lilac
Copy link
Contributor

lilac commented Jul 24, 2013

@huonw In Sydney too.

Your suggestion does work! I'll make a pull request.

@nikomatsakis
Copy link
Contributor

I was thinking about this some more. It's not clear to me that lifetime names should not be subject to hygiene. In the language as it stands, it's probably ok, but in the future we'd discussed the possibility of explicitly labeling the lifetimes of subexpressions and so forth, and in that case hygiene seems relevant. Of course maybe we can cross that bridge when we come to it.

@jbclements
Copy link
Contributor Author

Yep, makes sense to me. That was pretty much my thinking, as well, including the "cross that bridge when we come to it."

@tautologico
Copy link
Contributor

Was this reverted? I still see ident: Ident in ast::Lifetime

@huonw
Copy link
Member

huonw commented Nov 2, 2013

@tautologico, it never landed, there were some issues on windows that stopped it at the time, and I don't think @lilac resurrected their branch ever.

@jbclements
Copy link
Contributor Author

That's a shame; if I recall correctly, this was blocked on the same windows issue that was blocking my hygiene pull request--it later disappeared without any action on my part, allowing my patch to land. This one may now be viable as well.

@huonw
Copy link
Member

huonw commented Feb 22, 2014

I commented on #12451: #12451 (comment)

Summary: it seems that if we ever get macros that can expand directly to methods (#4621), then we will want lifetime-hygiene.

bors added a commit that referenced this issue Feb 23, 2014
@bors bors closed this as completed in 7607332 Feb 23, 2014
flip1995 pushed a commit to flip1995/rust that referenced this issue Oct 21, 2021
Add `format_in_format_args` and `to_string_in_format_args` lints

Fixes rust-lang#7667 and rust-lang#7729

I put these in `perf` since that was one of `@jplatte's` suggestions, and `redundant_clone` (which I consider to be similar) lives there as well.

However, I am open to changing the category or anything else.

r? `@camsteffen`

changelog: Add `format_in_format_args` and `to_string_in_format_args` lints
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue.
Projects
None yet
6 participants