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

undocumented lifetime elision fix from 1.15 -> 1.16 ? #41105

Closed
m4b opened this issue Apr 6, 2017 · 6 comments
Closed

undocumented lifetime elision fix from 1.15 -> 1.16 ? #41105

m4b opened this issue Apr 6, 2017 · 6 comments
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-core Relevant to the core team, which will review and decide on the PR/issue.

Comments

@m4b
Copy link
Contributor

m4b commented Apr 6, 2017

Specifically, this line of code failed to compile on 1.15.1, but successfully compiles on 1.16: https://github.com/m4b/goblin/blob/8188a8a15017cbdadfdbb807e08db968f1c634c9/src/mach/exports.rs#L94

For context, here is the important snippet:

impl<'a> ExportInfo<'a> {
    /// Parse out the export info from `bytes`, at `offset`
    pub fn parse(bytes: &'a [u8], libs: &[&'a str], flags: Flag, mut offset: usize) -> error::Result<ExportInfo<'a>> {
        use self::ExportInfo::*;
        let regular = |offset| -> error::Result<ExportInfo> {
            let address = bytes.pread::<Uleb128>(offset)?;
            Ok(Regular {
                address: address.into(),
                flags:   flags
            })
        };
        let reexport = |mut offset| -> error::Result<ExportInfo> {
            let lib_ordinal: u64 = {
                let tmp = bytes.pread::<Uleb128>(offset)?;
                offset += tmp.size();
                tmp.into()
            };
             // THIS line has lifetime issues on 1.15
            let lib_symbol_name = bytes.pread::<&str>(offset)?;
            let lib = libs[lib_ordinal as usize];
            let lib_symbol_name = if lib_symbol_name == "" { None } else { Some (lib_symbol_name)};
            Ok(Reexport {
                lib: lib,
                lib_symbol_name: lib_symbol_name,
                flags: flags
            })
        };

So, the fix (for 1.15) is adding explicit lifetime annotation to the closure return (e.g., let reexport = |mut offset| -> error::Result<ExportInfo<'a>>{..})

Consequently, it seems that this is an undocumented lifetime elision fix, or substantially less likely, 1.16 incorrectly compiles invalid lifetime code in closures.

I suspect its the former.

So, I dunno why, maybe I'm being an alarmist, but I find this somewhat serious, and fairly annoying; the only reason I upgraded to 1.16 was that I wanted cargo check for development, but upstream projects using my crate use 1.15 for CI, recommended version, etc; consequently, when 1.16 was released I noted that there were no major compiler fixes in this cycle, or major issues in compatibility notes, etc., which would cause a difference between a client compiling on 1.15 versus 1.16, so it should be safe to develop on 1.16 stable for cargo check; but alas, this is not the case.

@arielb1
Copy link
Contributor

arielb1 commented Apr 6, 2017

Can you provide a small example?

@m4b
Copy link
Contributor Author

m4b commented Apr 6, 2017

Unfortunately I don't have time right now. Compiling goblin 0.0.9 with 1.15 and 1.16 illustrates the issue. I suspect it's related to closures so a simple lifetime based struct + a closure may illustrate but I don't know

@Mark-Simulacrum
Copy link
Member

This was most likely caused by #39305. @brson is it worth adding the compatibility note to the 1.16 release notes now? I assume not, since we're on 1.17 (and soon 1.18) stable already...

@Mark-Simulacrum
Copy link
Member

cc @rust-lang/core -- should we add a note (and possibly backport?) to the 1.16 release notes?

@Mark-Simulacrum Mark-Simulacrum added I-nominated T-core Relevant to the core team, which will review and decide on the PR/issue. labels Jun 23, 2017
@nikomatsakis
Copy link
Contributor

This is certainly a result of #39305, or at least that suite of changes. As part of that, @eddyb made expansion within function bodies more likely to use inference variables, and hence more accepting -- and I suspect that is why this code is being accepted now. I think a compatibility note (or feature note, depending on your POV) would make sense. Something like:

Reimplemented lifetime elision. This change was almost entirely compatible with existing code, but it did close a number of small bugs and loopholes, as well as being more accepting in some other cases (e.g. https://github.com/rust-lang/rust/issues/41105).

@Mark-Simulacrum
Copy link
Member

I'm going to denominate and tag with E-easy. We should add the note to master's release notes in version 1.16, but I don't think we should bother backporting.

@Mark-Simulacrum Mark-Simulacrum added E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. C-enhancement Category: An issue proposing an enhancement or a PR with one. and removed I-nominated labels Jul 27, 2017
tamird added a commit to tamird/rust that referenced this issue Aug 20, 2017
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Aug 22, 2017
…ichton

RELEASES.md: document 1.16 lifetime elision change

Closes rust-lang#41105.

r? @Mark-Simulacrum cc @nikomatsakis
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. T-core Relevant to the core team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants