Contributor

Contributor

We just moved from try! to ? which greatly improves readability. I'd be sad for this movement backwards.

Alternative suggestion: add compiler support for forced MIR inlining of functions + some new MIR rvalue that gets turned into the function/line/col position of the inline location during such a forced inlining. This way no user code needs to be changed and we get the same advantage.

Contributor Author

We just moved from try! to ? which greatly improves readability. I'd be sad for this movement backwards.

Well, it doesn't effect code that uses try!/? at all. It means that foo.unwrap() would instead be written unwrap!(foo) which, to me at least, isn't any less readable.

Alternative suggestion: add compiler support for forced MIR inlining of functions + some new MIR rvalue that gets turned into the function/line/col position of the inline location during such a forced inlining. This way no user code needs to be changed and we get the same advantage.

How would that look? Would the unwrap/expect methods need to be written directly in MIR? Or would you add a macro to get the caller context? ie. something like:

#[inline(always)]
fn unwrap(self) -> T {
    match self {
        Some(t) => t,
        None => {
            let ctx = caller_context!();
            panic!("unwrap on empty Option at location: {}", ctx);
        },
    }
}

Contributor

It means that foo.unwrap() would instead be written unwrap!(foo) which, to me at least, isn't any less readable.

And code that uses foo.boo().unwrap().bar().blub().unwrap().cake() will be unwrap!(unwrap!(foo.boo()).bar().blub()).cake().

How would that look?

Should be possible the way you described it. Maybe even more like the asm! macro:

let line: u32;
let col: u32;
mir!(line = Inline(Line); col = Inline(Col));

Contributor

Another alternative (from C++) is default function arguments¹ (if they are evaluated at point of use):

fn unwrap(self, context: Context = file_line_column!()) { .... }
foo.unwrap(); // desugared into foo.unwrap(file_line_column!())

¹_{Which of course have numerous other applications.}

Contributor Author

@petrochenkov That doesn't really work. The file_line_column!() macro would have to be expanded at the point of the function declaration.

Member

What specifically is the issue with RUST_BACKTRACE=1 other than "you have to know to use it"?

Contributor

@canndrew

The file_line_column!() macro would have to be expanded at the point of the function declaration.

Then file_line_column need to be a compiler intrinsic or lang item or whatever works.

Contributor

Macros in method position would be nice for this too foo.unwrap!()

Contributor

@BurntSushi: The issue with RUST_BACKTRACE is that, depending on the platform, it may not provide the information you want: On OS X I'll only get the (mangled) function names and as far as I known in some windows configurations you don't even get that much.

Member

@TimNN Is that a deficiency in the current implementation that can be fixed?

Contributor

@BurntSushi even if it is fixed, if compiling in release mode, the backtrace might not yield any info that helps you, if enough functions got inlined.

Contributor

@BurntSushi I probably can be fixed and is tracked at rust-lang/rust#24346, to quote

So what is the plan with this? Is there any ETA?

Someone who feels sufficiently motivated will have to track down the appropriate libraries/APIs on Windows, OSX, BSD, etc and integrate them.

Contributor

I've wanted this before too, so I'm in favor of the basic idea. Have not read the RFC though. I'd much prefer if the macro was in method position though. The 'unwrappiness' is not the most important part of the whole expression, would rather not have it in front.

Contributor

I'm also very interested in distinguishing the 'expect' case from the 'sloppy coding' case. IOW, today I write 'unwrap' when I'm being sloppy, and 'expect' (often with an empty string) to mean, "I've thought about this and unwrapping is correct here". I would not want a single macro to mean both.

Contributor Author

today I write 'unwrap' when I'm being sloppy, and 'expect' (often with an empty string) to mean, "I've thought about this and unwrapping is correct here".

Why would you use expect with an empty string? I agree that expect is less sloppy but only because it forces you to provide a string justifying why it's safe to unwrap here. If you're passing in an empty string you may as well just use unwrap.

Contributor

Why would you use expect with an empty string? I agree that expect is less sloppy but only because it forces you to provide a string justifying why it's safe to unwrap here. If you're passing in an empty string you may as well just use unwrap.

expect vs unwrap signals intent. When I write expect, whether or not I give justification, I am asserting to future readers that this is not going to fail. If I write unwap then future readers have no indication that I wasn't just lazy. Granted, a message is better, but there's a clear semantic distinction between good unwrapping and bad unwrapping either way.

Contributor

@petrochenkov was onto something, although this would have to be done with type parameters in Rust:

impl<T> Option<T> {
    fn unwrap<C: SourceContext = CallerSourceContext>(self) -> T {
        match self {
            Some(t) => t,
            None => {
                panic!("unwrap on empty Option at location: {}", C::default());
            }
        }
    }
}

CallerSourceContext would be a lang item that the compiler replaces with a type implementing SourceContext which has some sort of API to get the information out.

I've been told Haskell does something similar with type parameters. cc @solson

What specifically is the issue with RUST_BACKTRACE=1 other than "you have to know to use it"?

One problem is when you run your application normally, and it crashes unexpectedly in a not immediately reproducible way. Now you have to rerun with RUST_BACKTRACE over and over again until you can find a way to reproduce the crash before you can get source information about where the crash happened.

Another prior art crate is the inner crate, which contains the inner! macro. It can do both unwrap! and try! functionality, and more.

Contributor Author

What specifically is the issue with RUST_BACKTRACE=1 other than "you have to know to use it"?

One problem is when you run your application normally, and it crashes unexpectedly in a not immediately reproducible way. Now you have to rerun with RUST_BACKTRACE over and over again until you can find a way to reproduce the crash before you can get source information about where the crash happened.

Exactly. And anyway, what's the point of printing some random location inside libcore when we could instead print somewhere useful.

@petrochenkov was onto something, although this would have to be done with type parameters in Rust:

Interesting, but I'm a little uncomfortable that CallerSourceContext looks like a type there but it's really some weird thing that gets substituted with a different, unique type everywhere that it gets used.

Member

And anyway, what's the point of printing some random location inside libcore when we could instead print somewhere useful.

This is a rather small and bearable cost IMO (in the context of RUST_BACKTRACE output).

One problem is when you run your application normally, and it crashes unexpectedly in a not immediately reproducible way. Now you have to rerun with RUST_BACKTRACE over and over again until you can find a way to reproduce the crash before you can get source information about where the crash happened.

I can appreciate that this is indeed a benefit.

As a data point, we're having problems getting good crash reports in Servo because .unwrap() doesn't give us the file/line number when it fails. We get the backtrace, but it's often not much use because of inlining and other optimizations.

Contributor

That can be fixed without changing the API, though. It might involve ugliness like adding a custom calling convention for unwrap-like functions (which would, of course, remain unstable forever), but I'd rather that than adding an unwrap! macro just for the good line info.

Not to say the format string isn't cool, though.

I think the line info is very my wanted.

Contributor Author

I'm kinda surprised by some of the negativity towards this. @notriddle would rather introduce a new calling convention then use a macro, @oli-obk would rather use asm! style hackery then use a macro, @BurntSushi would rather just go without accurate line info than use a macro.

What's so bad about using a macro? Is it just because it goes in-front rather than in the method position? That seems like a very minor complaint to me.

Contributor Author

expect vs unwrap signals intent. When I write expect, whether or not I give justification, I am asserting to future readers that this is not going to fail. If I write unwap then future readers have no indication that I wasn't just lazy. Granted, a message is better, but there's a clear semantic distinction between good unwrapping and bad unwrapping either way.

@brson would you rather there was both an unwrap! macro and an expect! macro? We could do that but AFAICT the two macros would do exactly the same thing.

Contributor

@canndrew Existing code gets 0 benefits with a macro, and you have to turn every caller into a macro too if you want to get the benefits transitively.
The type-system or calling convention "hacks" actually solve those problems.

Contributor Author

Well it's not ideal that there'd be two ways of doing things - the old way and the new way. But adding a new calling convention still seems like a heavy-handed way of solving something that we already have a solution for.

Member

I wonder if it is possible to have an #[inline(always_at_call_site)] attribute, such that the file!(), line!() and column!() macros inside the function will refer to the span at the call site instead of the point of macro expansion. We could then apply #[inline(always_at_call_site)] to unwrap(), expect() and unwrap_err().

This would require the result of these three macros remain non-constant until the inline-always pass though (or becomes an expression referring to the CallerSourceContext), which could be a huge hack.

Contributor

@kennytm That would work, but realistically it would have to be a MIR transformation with some way (other than the file/line/column macros, unless those are made magical) to get the source information.

Sorry, I didn't see the last part of your comment. Yes, that makes sense in the context of a MIR inlining pass.

If we had a macro loc! which appended the file/line/column number, but otherwise acted like format! we could get a lot of the benefit of this just by writing .expect(loc!()) in place of .unwrap(). If we were allowed to elide empty arguments, this would be .expect(loc!).

Contributor

If we had a macro loc! which appended the file/line/column number, but otherwise acted like format! we could get a lot of the benefit of this just by writing .expect(loc!()) in place of .unwrap(). If we were allowed to elide empty arguments, this would be .expect(loc!).

That wouldn't work with existing code, and it would be harder to use than a plain unwrap! macro.

@notriddle true, it wouldn't help with existing code. Not sure about ease of use, e.g. compared to worrying about the order of inlining vs macro expansion.

Contributor

That should be transparent to the user; the only people who should have to worry about that are the libs and compiler teams.

Contributor

@brson would you rather there was both an unwrap! macro and an expect! macro? We could do that but AFAICT the two macros would do exactly the same thing.

Yes.

Contributor

use asm! style hackery

Adding a mir! macro sounds reasonable to me anyway $someday, so we could add a hacky internal version now, and worry about doing it correct later, as it would only be used inside the stdlib anyway.

This would require the result of these three macros remain non-constant until the inline-always pass though (or becomes an expression referring to the CallerSourceContext), which could be a huge hack.

I don't think it's hacky or unreasonably complex to add code that will be transformed by the inline-always pass. We'll have many transformations on MIR in the future, this will be a a simple one-way transformation. I think the actual inlining pass will be more complex than the expansion of the macro to the caller's function_name/file_name/line/col (which should mostly just be "is this a mir!?" -> yes -> "is this a source context mir!?" -> yes -> replace it by a statement moving the values as constants into the given target)

Contributor

My 2c:

I agree with many on the thread that:

• The lack of useful line info is a real problem today. Even though we can continue to make improvements to the visibility of backtraces, the reality is that it's a cumbersome and not newbie-friendly experience.
• OTOH, as @eddyb points out, solving the problem by introducing a new means of unwrapping is problematic because it doesn't help with any existing code. At this point the conventions around unwrap are quite well-settled, and moving such a core operation to a macro seems unfortunate.

In general, I suspect that building consensus around macros as the stabilized solution to this problem is an uphill battle. I wonder if it's worth branching off a thread on internals to talk about other ways of solving this problem within unwrap itself, as some are already doing here?

Member

🔔 This RFC is now entering its week-long final comment period 🔔

The libs team is leaning towards closing this along the line of @aturon's previous comment. It seems too late in the game to shift conventions as .unwrap() is so entrenched. We agree though that this is a very real problem and we'd like to tackle it, but it probably needs to be done through a different means than a new macro.

Contributor Author

I meant to close this already but I've been very busy. Here's the internals discussion thread as per @aturon's suggestion.