Rustup to *rustc 1.10.0-nightly (764ef92ae 2016-05-19)* and bump to 0.0.69

[mcarton]

Ref rust-lang/rust#33654.
Fix #945. Fix #946.

[mcarton]

Don’t know why the tests are failing 😓

[oli-obk]

Some are failing, because there's no way to detect that a for loop was expanded, except to check the HIR for blocks that state that they are for loop expansions. But the HirMap::parent method doesn't give the actual parent if run on anything inside the expansion, but it gives the parent of the for loop.

[oli-obk]

ahaa, this probably occurrs because the idents in HIR don't carry information about expansions anymore, but are just names now: rust-lang/rust#33654

[oli-obk]

more info: rust-lang/rust#30145 (comment)

Besides name resolution unhygienic_name is used in some lints and debuginfo. unhygienic_name can be very well approximated by "reverse renaming" token::intern(name.as_str()) or even plain string name.as_str(), except that it would break gensyms like iter in desugared for loops. This approximation is likely good enough for lints and debuginfo, but not for name resolution, unfortunately (see #27639), so unhygienic_name has to be kept.

[oli-obk]

probably this should be gensym("_result") instead it is already forwarded to gensym

[oli-obk]

or even better, it should simply not create a local variable at all

[mcarton]

ahaa, this probably occurrs because the idents in HIR don't carry information about expansions anymore, but are just names now: rust-lang/rust#33654

I know, I linked that in the PR 😄
I meant “I don’t know why name.unhygienize() seems different from ident.unhygienic_name here”.

probably this should be gensym("_result") instead
or even better, it should simply not create a local variable at all

That local variable is weird, but I can’t update rustc 😅

We might need to rewrite the lint to find that out otherwise but we can’t make it an early-lint because there seem to be no way to get a syntax::ast expression’s parent.

@Manishearth ?

[oli-obk]

I know, I linked that in the PR 😄

whoops :)

That local variable is weird

it's there due to some lifetime issues with putting values into the tail expression

I meant “I don’t know why name.unhygienize() seems different from ident.unhygienic_name here”.

Well... unhygienize uses the thread local interner, which is cleared several times before HIR lints run, iff there is a previous compilation error 1, 2

Easy fix: don't run lints if there were previous errors

nevermind, they are simply cleared unconditionally xD I'll file a PR to move the clearing after lint checks

that's not it either

[Manishearth]

That local variable is weird, but I can’t update rustc

sure you can 😄

[Manishearth]

We could switch over UsedUnderscoreBinding to use a regular macro check and switch the shadow pass to ignore macros too.

[oli-obk]

We could switch over UsedUnderscoreBinding to use a regular macro check and switch the shadow pass to ignore macros too.

temporarily? because if permanent, we loose the checks if the name is used in a macro like println!("{}", _something);

[Manishearth]

I'm not sure if we do?

[oli-obk]

I'm not sure if we do?

Maybe, but the issue with the for loop won't go away, since it's not from a macro expansion, but from ast -> hir lowering, which isn't recorded.

[mcarton]

With in_macro we lose println!("{}", _foo); and _result in for loop desugaring still warns.

[petrochenkov]

Why isn't unhygienize() used in this file?

[mcarton]

I does now, but the lint still has problems.

[petrochenkov]

One more missing unhygienize() here.

[petrochenkov]

If some lints don't work due to gensyms after proper replacement of all unhygienic_names with unhygienize() , they need to be moved to AST.

[Manishearth]

Fair point.

[mcarton]

One more missing unhygienize() here.

Stupid me. That lint seems ok now. Thanks.

they need to be moved to AST

I would have done that but used_underscore_binding needs to get the parent of an expression. With hir that’s in rustc::hir::map::Map, there does not seem to be an ast equivalent. I’ll see if we can rewrite the lint to not use that.

[mcarton]

Another day, another problem:

    with_vec.unwrap_or(vec![]);
    //~^ERROR use of `unwrap_or`
    //~|HELP try this
    //~|SUGGESTION with_vec.unwrap_or_else(|| vec![]);

is now reported as:

tests/compile-fail/methods.rs:        with_vec.unwrap_or_else(|| < [ _ ] > :: into_vec ( box [ $ ( $ x ) , * ] ));

This looks to be because of rust-lang/rust#33730 or rust-lang/rust#33712. Looks like snippet won’t work with macros anymore, we could get the right span with CodeMap::source_callsite I guess, but how would we know when we have to use it?

[Manishearth]

@jseyfried @nrc Tips on what to use in place of snippet here?

[Manishearth]

Am I right in assuming that the broken snippets are the only thing left to fix? In that case, we could temporarily patch the tests and fix this later.

[mcarton]

Am I right in assuming that the broken snippets are the only thing left to fix?

No, used_underscore_binding still warns on loop desugaring _result variable.

[Manishearth]

We can quick patch that to ignore variables with that name too.

[mcarton]

After 3 rustups the tests are finally passing \o/
I made used_underscore_binding allow-by-default and ignore _result for now. Also I commented out a macro-span test.
@Manishearth are you ok with that? Can we publish?

[Manishearth]

Yep, but file follow-ups for the corners we cut. I'll poke at it later.

[jseyfried]

@Manishearth

Tips on what to use in place of snippet here?

This was caused by #33712, which fixed the span of expanded expressions that are the root of their expansion. These expressions used to have the span of their expansion's invocation (the call_site).

Note that expanded items have always had the correct span, for example:

macro_rules! m {
    () => { const fn f() {} } //< This is the span of the expanded item, ...
}

m!(); //< ... not this.

The most direct way to fix the fallout here is to compute what the span would have been the change:

fn expansion_root_to_call_site(self: &CodeMap, mut span: Span) -> Span {
    self.with_expn_info(span.expn_id, |info| info.map(|info| {
        if info.callee.span == Some(span) { // Is `span` an expansion root?
            span = self.expansion_root_to_call_site(info.call_site); // note the recursion
        }
    }));
    span
}

I think you could also just use source_callsite. souce_callsite(span) is expansion_root_to_call_site(span) iff expansion_root_to_call_site(span) has NO_EXPANSION, which I believe is the case whenever we report the unwrap_or lint -- it appears we already avoid linting expanded code.

With expanded spans, expansion_root_to_call_site never makes things worse but does not always lead to a usable snippet, which may not exist.