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

Add an unwrap! macro #1669

Closed
wants to merge 1 commit into from

Conversation

Projects
None yet
@canndrew
Copy link
Contributor

commented Jul 7, 2016

Rendered

@canndrew canndrew force-pushed the canndrew:unwrap_macro branch 2 times, most recently from 18188ce to c37a267 Jul 7, 2016

@canndrew canndrew force-pushed the canndrew:unwrap_macro branch from c37a267 to 26001f3 Jul 7, 2016

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

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.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

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);
        },
    }
}
@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

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));
@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

Another alternative (from C++) is default function arguments1 (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!())

1Which of course have numerous other applications.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 7, 2016

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

@BurntSushi

This comment has been minimized.

Copy link
Member

commented Jul 7, 2016

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

@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.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

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

@TimNN

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

@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.

@BurntSushi

This comment has been minimized.

Copy link
Member

commented Jul 7, 2016

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

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

@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.

@TimNN

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

@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.

@alexcrichton alexcrichton added the T-libs label Jul 7, 2016

@brson

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

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.

@brson

This comment has been minimized.

Copy link
Contributor

commented Jul 7, 2016

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.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 8, 2016

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.

@brson

This comment has been minimized.

Copy link
Contributor

commented Jul 11, 2016

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.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 11, 2016

@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

@solson

This comment has been minimized.

Copy link
Member

commented Jul 12, 2016

@eddyb What I told you about before (I think) was the HasCallStack typeclass (note that it has no parameter, which isn't directly possible in Rust since all traits have Self).

Another relevant thing I found uses GHC implicit parameters to provide call site location information, like your example: https://ghc.haskell.org/trac/ghc/wiki/ExplicitCallStack/ImplicitLocations

Implicit parameters themselves are described at https://www.haskell.org/hugs/pages/users_guide/implicit-parameters.html, but call site locations are a compiler-magic special case of them.

@crumblingstatue

This comment has been minimized.

Copy link

commented Jul 12, 2016

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.

@diwic

This comment has been minimized.

Copy link

commented Jul 12, 2016

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

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 13, 2016

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.

@BurntSushi

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

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.

@asajeffrey

This comment has been minimized.

Copy link

commented Jul 13, 2016

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.

@notriddle

This comment has been minimized.

Copy link
Contributor

commented Jul 13, 2016

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.

@wuranbo

This comment has been minimized.

Copy link

commented Jul 15, 2016

I think the line info is very my wanted.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

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.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

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.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

@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.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 15, 2016

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.

@kennytm

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

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.

@eddyb

This comment has been minimized.

Copy link
Member

commented Jul 15, 2016

@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.

@asajeffrey

This comment has been minimized.

Copy link

commented Jul 15, 2016

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!).

@notriddle

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

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.

@asajeffrey

This comment has been minimized.

Copy link

commented Jul 15, 2016

@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.

@notriddle

This comment has been minimized.

Copy link
Contributor

commented Jul 15, 2016

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

@brson

This comment has been minimized.

Copy link
Contributor

commented Jul 18, 2016

@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.

@oli-obk

This comment has been minimized.

Copy link
Contributor

commented Jul 20, 2016

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)

@aturon aturon self-assigned this Jul 22, 2016

@aturon

This comment has been minimized.

Copy link
Member

commented Jul 22, 2016

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?

@aturon aturon added the I-nominated label Jul 25, 2016

@alexcrichton

This comment has been minimized.

Copy link
Member

commented Jul 26, 2016

🔔 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.

@canndrew

This comment has been minimized.

Copy link
Contributor Author

commented Jul 28, 2016

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

@canndrew canndrew closed this Jul 28, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.