RFC: Implicit caller location (third try to the unwrap/expect line info problem)

Current (Implicit caller location)

Rendered
Proof of concept

Legacy (Semantic inlining):

Rendered
Proof of concept

We could change the meaning of file!(), line!() and column!() so they are only converted to real constants after semantic-inlining (a MIR pass) instead of early during macro expansion (an AST pass). Inside #[inline(semantic)] functions, these macros behave as this RFC's core::caller::{FILE, LINE, COLUMN}. This way, we don't need to introduce panic_at_source_location!(). The drawback is losing explicit control of whether we want to report the actual location or the caller's location.

I think the macros should always resolve to the special lang item, and the lang items should also work in non-inlined functions (but you'd get the location of the lang item itself, not the caller). And maybe the lang items could be annotated such that they can be used from within the macros but not written manually.

Hmm. Turning file!(), line!() and column!() just into special consts will break backward-compatibility. They need to expand into special literals (or concat! needs to pretend those lang items are literals).
const FILE: &str = file!();
const LINE: u32 = line!();
macro_rules! my_file { () => (FILE) }
macro_rules! my_line { () => (LINE) }
fn main() {
    println!("{}", concat!(file!(), ":", line!()));
    //^ Fine, prints src/main.rs:6
    println!("{}", concat!(my_file!(), ":", my_line!()));
    //~^ ERROR: expected a literal
}

Uh oh. That might be unintended. At least the docs say it expands to an expression of type &'static str, not a literal. But I suppose we can't change that now.

@jethrogb Actually expanding to a literal would be far easier than a special const item. We just add a new LitKind and make file!() and friends (which are procedural macros) the only way to generate such LitKind.
However, doing so would make panic not able to use the static _FILE_LINE_COL = (file!(), line!(), column!()); code-size optimization trick.

Is that optimization worth it? It only means the value is not duplicated between crates.
&(file!(), line!()) hasn't had the values on the stack since before 1.0 (rvalue promotion to 'static - you still need a feature gate to get the borrowck to believe it but the rest of the compiler does it anyway as an optimization) - and we deduplicate constants like that within a crate already.

I just want to say that this is one of the most well-written RFCs I've ever seen. Amazing job @kennytm !

First, thanks @kennytm for writing such a well written RFC! It is definitely important to fix this issue of the unwrap function.
First on a matter of coordination. @japaric has opened RFC #2070 recently which concerns enabling panic! to work on no-std applications without using unstable features. As the two RFCs both affect the panic mechanism of Rust, there is some level of overlap here, it would be great if you both could coordinate to make sure the RFCs are compatible with each other.
Semantic inlining vs traditional inlining
I want to emphasize what @sfackler has pointed out above: #[inline(semantic)] is different from traditional inlining. It violates the usual boundary between the calling and the called function to a greater extent than traditional inlining. With traditional inlining, you usually want to retain the separation between the caller and the calee in the stack trace and get annoyed if the compiler/debugger mixes them up, and with #[inline(semantic)] you want the exact opposite. Teaching people that this is the same mechanism will only cause confusion IMO.
Due to the point above, the #[inline(semantic)] name is much greater than the alternatives mentioned. I especially like how "semantic" conveys the meaning that there is an actual "flow of data" from the caller to the callee (the location info), independent from parameters passed. I wouldn't like a rename to #[inline(mir)] because right now, there is no mention of MIR in the language anywhere, and I'd say its just an implementation detail, which it should IMO stay. #[inline(force)] sounds too similar to #[inline(always)], while both are semantically different things.
If the #[inline(semantic)] name can be improved, then by making it less close to the traditional inlining mechanism.
location info lang items
As the file!() macro and friends can't expand to simple literals any more, the RFC proposes the addition of lang items that get classified as constants, but get special meaning as e.g. constant folding must ignore them. They essentially get treated like dummies until the MIR inlining pass. The basic problem here is that you need to have some dummy that the MIR semantic inlining pass can expand to a literal, but which can also be expressed by the token stream API that macros use as input and output. The RFC proposes to solve this via addition of lang items, which is a great way of doing this.
lang items get treated specially by the compiler, so they are always more than just what you call them. You still need to find some category in the existing language where the a feature fits in really well, which you then use to expose it to the remainder of the language. I think the category chosen right now, constants, is not really suiting. For me, an important property of constants is that they are a never changing value, everywhere the same. There are mutable statics, but this is a different thing obviously. Also, you'll have to add an exception to constant folding to specifically avoid them, and you need to define dummy values at the declaration site that might be mistaken by the reader for actual values.
A much greater way of categorizing the lang items would be intrinsic functions marked with #[inline(semantic)]. Intrinsic because they will be implemented in the compiler, and #[inline(semantic)] because the the context of the calling place will affect their evaluation (Of course, it is natural that a call to an intrinsic function gets inlined, but marking it #[inline(semantic)] gives us that additional meaning).
Forwards compatibility
When I added column info to panic printing, I was pleasantly surprised that panic info printing was forwards compatible to a great degree; I didn't have to avoid breaking any stable API. This property should be preserved, even though column info is being printed already.
Regardless of whether the actual change is a good idea or not, I think its a good property to support future changes to e.g. include more information from the span, like e.g. the ending line and colum, or crate names, or similar.
The panic hook mechanism has the opaque Location struct which is very forward compatible. If we add a caller_location macro, it should return this struct instead of a tuple.
I haven't found any other places where this RFC is incompatible with future extensions, but if there are any, it would be great to get rid of them.

I certainly agree this is annoying problem, and one worth solving. It seems to me that the big decision to be made is indeed whether to assume that debuginfo is not a viable path -- I believe that other languages have solved this by having the ability to annotate functions as being "implementation details", and hence these functions get omitted or overlooked in a backtrace (i.e., by the backtrace machinery). But I can't find any citations for this second approach.
In a way, the two are similar. The idea is to label functions that users probably don't want to see. I think one could argue that implementing this via inlining is an 'implementation detail', right? The important bit is that the various new macros in question will work give the position info from the caller, right? (Or are there other things I am overlooking where it is crucial that the function is inlined at the MIR level?)

Or are there other things I am overlooking where it is crucial that the function is inlined at the MIR level

It is important that the function is inlined in MIR instead of waiting for LLVM because the location info is separate for each time a function gets inlined. We want to expand the location info to a literal at compile time and not read it from debuginfo at runtime so that this works with builds whose debuginfo is stripped. IMO we should still offer an option to strip even this info (I think right now its possible to do that via custom panic_fmt lang items, not sure though), but there should be a mode where main debuginfo is being stripped, but the panic location is still being printed.

Inlining is so painful to debug, I'd like to avoid it here if at all possible.
I also don't really agree with your rationale that relying on debug info is not an option. In my experience, the location information from a panic message is rarely useful (at all) in trying to figure out what went wrong - you need at least a backtrace for that.
I just don't see the value in trying to provide this information even when there's no debug info. It's certainly of no immediate use to end users, but I can imagine it being relevant for bug reports. However as I said, a bug report without a backtrace is rarely useful anyways.
From this perspective, I would suggest a very different approach: Rely on debug info entirely. Then, the solution to this problem can be as simple as just always printing more than a single stack frame (say 3) by default (having to re-run from the beginning with RUST_BACKTRACE=1 is always very annoying).
Release builds are of course an issue but I believe there's a much better solution for that: Build with debug symbols but save them externally, in their own file. This also means that bug reports from end users are now much more useful overall as the backtrace can be decoded using the symbol file.
Anyways, that's just what I'd like to see from my personal experience.

@est31

It is important that the function is inlined in MIR instead of waiting for LLVM because the location info is separate for each time a function gets inlined.

Can inlining be replaced with a special ABI with modified calling conventions using additional implicit argument for passing (pointer to) the location?
When such functions are converted into function pointers some shims will have to be created though.

I like this idea of MIR inlining a lot. As for another use case: It would be nice if the callee could be able to verify, at compile-time, that the caller is eligible, given some policy or otherwise.
I'd also like to see an implementation of this panicking business where this source location data could be stripped from the binary. Points of possible panic are rather prevalent.

@eddyb @kennytm
Could you elaborate why the ABI tweak is a bad alternative compared to inlining?

@petrochenkov It's not as bad as it is involving in the backend. Unless you mean it's all in the Rust compiler, which could make it a MIR pass, which, uhh, would be vaguely equivalent to inlining.
If it's not a new calling convention but just an extra argument rustc adds, that's fine.

Yeah, I meant normal calling conventions (in low level sense), just an extra argument implicitly added during translation.

Thanks for the feedback 🙂 Long post below.

@est31 on Forwards compatibility
The current PoC did not change the how panic works besides relaxing the lifetime requirement of core::panicking::panic, so there should be no problem with future changes of Location and #2070.

@sfackler @est31 on Traditional inlining vs Semantic inlining, and Location info lang item
This RFC itself did not propose changing file!() as friends (yet). The actual file!() was still retained, e.g.
#[inline(semantic)]
fn boom() {
    panic!("kaboom");  // line 3
}
fn main() {
    boom(); // line 6
}

with the current implementation, the panic will still point to line 3. So this is still somewhat like traditional inlining.
To entirely blur the line between caller/callee, we need to change file!() to expand to a special literal (which has no source representation) as mentioned in the first 5 comments. It cannot be an intrinsic or const or static because concat!(line!()) or include_str!(file!()) outside of #[inline(semantic)] should not be affected.
Assuming we don't care about panic's optimization, the only hazard is that,
#[inline(semantic)]
fn foo() -> bool {
    const LINE: u32 = line!();  // <-- this will not be the caller's line
    let l = line!() // <-- this will be the caller's line
    l == LINE // usually false
}

The static/const should trigger an error something similar to E0401. There should also be file!(after_macro_expansion_but_before_inline_semantic) to provide the traditional behavior.
I'm open to keeping core::caller::* and change those into intrinsics, but I'll also see would going through the LitKind route be too dramatic a change. I may modify the RFC after that.

@nikomatsakis @main-- on Debuginfo
Disclaimer: I've never used a debugger on rust generated program for serious debugging because (1) printf-debugging is much more accurate and easier (2) lldb doesn't seem to work properly[a] (3) the only time I've used lldb is to obtain a more correct backtrace, since RUST_BACKTRACE=1 does not work at all on macOS (rust-lang/rust#24346). So I am certainly biased here 😆.
The biggest reason debuginfo is not reliable because it is often absent. And it is absent because it is not generated by default, is huge (>100% size of the executable) and leaks sensitive source code info for propriety programs. Look, even rustc does not ship with any debuginfo
$ find ~/.rustup -name '*.dSYM'

$

💭 Maybe rustup should provide an optional "debuginfo" component 😉

There is also the issue that RUST_BACKTRACE=1 is not enable by default, partly because it is slow (rust-lang/rust#42295), and also because sometimes your system just doesn't have libunwind and you can't even get one level of backtrace.
And I just feel that a systems language with thin runtime shouldn't require a runtime solution to get the caller info.

💭 BTW I wonder why our CI system does not include RUST_BACKTRACE=1? Currently we have logs like https://travis-ci.org/rust-lang/rust/jobs/259620982 (from rust-lang/rust#43506) which fixing requires guesswork.

Note: [a]: lldb "step" jumps around randomly, "print" often don't work probably because the variable is optimized away etc. Sorry I didn't try to reproduce them properly, so no issues filed.

@nikomatsakis on Omitting backtrace (a.k.a. #1744)
Yes implementing this via inlining is an implementation detail. And yes the important bit is the various lang items will work. But I disagree that the idea is to label functions that users don't want to see. In fact, if possible I want unwrap/expect to remain visible in the stack trace, like
thread 'main' panicked at 'called `Option::unwrap()` on a `None` value', 1.rs:7:4
stack backtrace:
  10: (garbage omitted for brevity)
  11: <core::option::Option<T>>::unwrap
  12: _1::main
  13: __rust_maybe_catch_panic
  14: std::panicking::try
  15: std::rt::lang_start
  16: main

This could be done by cloning the function's MIR + rewrite + redirect the caller, instead of inlining them. That is more aligned to @sfackler's comment that #[inline(semantic)] is parametrization by call site. But I can't find the way to add a Mir yet; a MirPass can only modify existing MIR. Anyway it is just an implementation detail.
Omitting part of backtrace inside an IDE is not a new idea, Xcode supported this very long time ago, even with dynamic granularity setting. But I'm not aware of any languages besides Swift that allow programs to annotate functions as unimportant themselves, at least not in Python or Java.
The problem isn't that the backtrace is too verbose, but it is not precise enough at the right place due to (traditional) inlining, or that the backtrace does not include the line number and you used more than one unwrap in the function.

@petrochenkov on Special ABI
It won't work because fn(T) and extern "abi" fn(T) are two different types. If they are the same type then it is no different from using an #[attribute].

I have used gdb on Rust a few times and I agree that it could use some improvement. The seemingly random jumps you're seeing are probably due to the fact that the debug symbols emitted by rustc tend to associate almost every instruction with the original statement that caused it to be generated. This is rather problematic as optimizations shuffle things around quite a bit.
But I consider this a bug that should be fixed one day (C/C++ compilers have a decent workaround).

I'm well aware that debug info is rarely shipped in releases. I even believe storing debug info externally (like Microsoft have been doing for ages with their pdb files) is very unconventional in the Unix world today (FWIW Ubuntu do provide symbol files for some of their packages). I wonder why? Is there any big drawback I'm not aware of?

The biggest reason debuginfo is not reliable because it is often absent. And it is absent because it is not generated by default, is huge (>100% size of the executable) and leaks sensitive source code info for propriety programs. Look, even rustc does not ship with any debuginfo

Yes, this is certainly an issue today. My suggestion was not "just use debuginfo", it was "make sure debug symbols exist (throughout the ecosystem), then just use that". Most notably:

You would not usually ship a symbol file to end users. In the case of proprietary software with binary distribution this means that it does not leak any information about your source code (not even the the file names and location numbers for panicking).
You should still be able to decode a backtrace from those end users on your side using the symbol files (e.g. in a bug report).
And in the case of open source software, a user that wants to debug the application can just download the symbol file as well and start debugging.

:thought_balloon: Maybe rustup should provide an optional "debuginfo" component :wink:

:+1: It should!
And again, my core argument here is basically that in a world where rustc generates symbol files for every release build by default, not having these symbols always means that showing line info is undesirable.

First of all, thank you for raising this issue and providing an avenue of discussion; it seems that whenever I write tests I have to have at least this one instance of Option::expect panicking and having to rerun the tests with backtracing enable to get a clue as to where the issue happened.

I think that to solve the optimization issue it could be useful to have a special flag/setting which strips column information from debug information and source locations (making them 0, or even removing them altogether).
This is something that gcc and clang have, with -glevel=1 (0: no debug, 1: minimal debug and 3: full debug). It is an interesting compromise to get some level of backtrace/source-level tracking without completely bloating the generated binary.

With regard to your proposal, I am not a fan of inlining. It seems to me inlining is limited and may cause issues:

does not work with recursive functions (mentioned in RFC, but should be added to Drawback section),
does not work with polymorphic calls,
poorly works with large functions.

The latter can be worked around by having a lint advising users to create a private function with the bulk of the implementation and then using the current public function as a forwarding shell (which can then be inlined without bloat). However it does mean shifting the burden onto the users.
As a result, much as did @petrochenkov, I too wonder whether a different ABI/magic parameter would not be a better solution.

As a strawman proposal, I'll go with the "magic parameter" approach:
#[ghost]
fn panic() -> ! {
    let loc = location!(); // magically retrieves the source location from the ghost argument
}

//  Exposed as parameter when taking a function to pointer, or converting to `Fn`.
static PANIC: fn(&'static ::core::source::Location) -> ! = panic as fn(&'static ::core::source::Location) -> !;

//  Can be passed as regular parameter, even though not part of "official" function signature.
panic(location!());

The essence here is that if we are to be magical, I don't see why we cannot have simili default parameters.
Calling a #[ghost] function from a #[ghost] function should either:

magically form a backtrace: would need an array of Location then, and unsure how it would work with polymorphic calls,
or simply forward the current location.

The latter is fully decidable at compile-time.
There is one question: how should specialization/implementation of a trait method work. There are two alternatives as far as I can see:

If the base method is #[ghost] then the specialization/implementation MUST be #[ghost] too; and vice versa.
The specialization/implementation being #[ghost] is independent of the base method:

If the base is #[ghost] and specialization/implementation is not, then the specialization/implementation ignores the magic argument (still passed for ABI reasons),
If the base is not #[ghost] and the specialization/implementation is, then when called statically it has the caller's location but when called dynamically it has its own location (essentially, two functions are generated).

I am not sure which is best; I would recommend starting with the first one to accumulate experience and check whether this is really an issue to start with.

@matthieu-m I have thought of magic parameter and tried to implement that 3 times but it involves too many parts of compilers to make it work 😄 (AST/HIR to split a function into two, and MIR to rewrite the call, probably something more which I forgotten).
Essentially #[ghost] would generate two functions,
#[ghost]
fn x(args: T) -> R { ... }

// =>

#[ghost(redirect="def_id of x$real")]
fn x(args: T) -> R {
    x$real(args, &Location::current())
}

fn x$real(args: T, location: &Location) -> R {
   ...
}

then, after type-checking, any direct call to x would be rewritten to a call to x$real.
#[ghost] cannot use the "and vice versa" option because I want Index::index be #[ghost] too, and that would break downstream source code. Allowing panic to accept an optional parameter is probably not good for inference, since this is basically function overloading which Rust avoids.
Some questions for this:

How should visibility be handled (the x$real usually comes from an external crate)
How to store the x ↔ x$real relationship (queries?)
How to do add the x$real without adding an item to the trait
What happens when I take a function pointer. Note that the type of Option::unwrap must be coercible to fn(Option<T>) -> T, it cannot be fn(Option<T>, &Location) -> T.
Should x be #[inline(always)]

After thinking a bit of these, I find that the effect is very similar to inlining in terms of what I want to solve (unwrap and index), and went with that instead.

@main--

I even believe storing debug info externally (like Microsoft have been doing for ages with their pdb files) is very unconventional in the Unix world today (FWIW Ubuntu do provide symbol files for some of their packages).

ELF-based systems like Linux and BSD do embed DWARF section in the binary. But macOS store them externally in a *.dSYM folder. MSVC uses *.pdb as you've mentioned.

ELF-based systems like Linux and BSD do embed DWARF section in the binary.

They traditionally do so in debug builds. And it makes sense there.
But moving them to a separate file is perfectly possible:
objcopy --only-keep-debug my_binary my_binary.sym
strip --strip-debug --strip-unneeded my_binary
objcopy --add-gnu-debuglink=my_binary.sym my_binary

This is what I would suggest for release builds on ELF systems.

@kennytm thanks for your reply. Could you also reply to these suggestions of mine, you seem to have missed them:

caller_location should return a Location struct, not a tuple, for forwards compatibility. You just said that your entire RFC is forwards compatible, but caller_location is clearly not.
replacing "magic constants" with #[inline(semantic)] intrinsic functions. Fits much better than constants.

@est31

I see, I meant #[inline(semantic)] is compatible with any change of core::panicking. caller_location returning Location would require either Location be moved into libcore (and core::panicking::panic_fmt changes to accept a &Location instead of tuples), or caller_location be moved into libstd. If caller_location is moved to libstd then Option/Result cannot use it.
If Location is moved to libcore and caller_location is changed from a macro to a function I'd prefer caller_location() be renamed as Location::caller(), this needs #[inline(semantic)] to report the location at the top of the inline stack though.

I've replied in the last paragraph in the 2nd section. Intrinsic is fine (they don't need to be #[inline(semantic)]), but I want to experiment with file!() expanding to special literal first.

Use of inline is very surprising to me. To me this feature is not related to inlining at all. It's implemented in the proof of concept by hacking inlining, but that should be left an implementation detail.
To me this feature is all about distinction between errors caused by the caller or the callee:

A function may panic because it got invalid input from the caller (the report location where the function was called from, since that where the bad input lives),
A function may panic because it created invalid state itself (the report location should point to inside the function, because that's where the bug lives).

The fact that it ties code generation to error reporting is unfortunate. I'd probably resort to:
fn calc(val) {
   if val < 0 {panic!("your fault");}
   actually_do_calc_without_checking(val);
}

In order to get correct panic location, and minimize unwanted inlining.

Regarding the Backtrace section in the RFC: IMHO this is benefit, not a downside.
When I strip release executables it's because I want to remove all the bloat, and hide all information about my sources. If that improves effectiveness of stripping, that's better.
In cases where I want to get accurate backtraces — and panics — I keep the debug symbols.

Hi all, I've rewritten the RFC and PoC based on the feedback.

The attribute is changed from #[inline(semantic)] to #[implicit_caller_location], to reduce the connection with traditional inlining.

Location information is now fed in by redirecting a call from foo(bar) to (conceptually) foo::real(bar, file!(), line!(), column!()), not by forcing inlining, thus the code size should be smaller.

Location is propagated automatically, thus recursion is now allowed.

The location is retrieved using the caller_location() intrinsic instead of magic constants.

panic!() no longer stores the file/line/column in a static variable. It takes the file/line/column using caller_location().

Added a -Z location-detail=none flag to inhibit caller_location() from giving real file/line/column. (Yes this affects normal panic as well)

Things not changed:

No, a separate ABI still does not work. I've expanded the RFC section to explain why this alternative is not chosen.

No, debuginfo alone (RUST_BACKTRACE=1) is still not a reliable way to get the location of a single frame IMO.

file!()/line!()/column!() still report their true location, not the caller's location.

We are still using the MIR inliner pass as implementation detail, thus:

Diverging functions are not supported
Trait items are not supported (still requires post-monomorphization transformation)

The signature of panic_fmt and friends are kept the same. Changing them is #2070's job.

New problems:

Using caller_location() on wrong items may not give you the correct location

Diverging functions... (issue of implementation detail)

No way to allow explicitly insert the location yet.

The #[inline]/#[cold]/#[naked] attributes are ignored.

The #[inline]/#[cold]/#[naked] attributes are ignored.

Theoretically, an implementation could error if it encountered both the #[implicit_caller_location] attribute and one of them?

@est31 Yes it could, just like how this is currently rejected for trait items.
(we could also use an inner fn instead of a closure, though the function signature would be a mess to deal with.)

This is such a well-written RFC but I still disagree with an important part.
Your rationale states:

Debug-info independence. The precise caller location can still be reported even after stripping of debug information, which is very common on released software.

Later on you elaborate:

For Rust, however:

The debug information is usually not provided in release mode.

I may sound like a broken record here, but I'm not aware of any practical reason why you would opt out of external debug info for release builds. Your RFC is already changing the compiler, so it really doesn't make sense to assume other parts are set in stone.
In Windows as well as Mac OS this is the standard and has been for many years. Yes, things are traditionally done differently in the Unix world but today's tools are already quite capable of handling this and as Ubuntu/Debian's packaging shows, things are moving in that direction as well.
With that in mind - as I already explained - debug information would be available to exactly those users who can make sense of it.

Normal inlining will make the caller information inaccurate.

Yes and no. Simple symbol-based backtracing implementations certainly miss inlined frames but it is of course possible for the compiler to annotate code from inlined functions with their original source information. In fact, rustc is already capable of doing this today - try it!

Issues rust-lang/rust#24346 and rust-lang/rust#42295 are still not entirely fixed.

Platform support is a valid drawback, I agree.

Backtrace may be disabled in resource-constrained environment.

I'm not sure if this is even a drawback. If anything, it might be a feature: In symbol-based panicking, your resource-constrained microcontroller no longer has to deal with source location information at all - it just prints a stack trace which you can then decode on another machine which has the full debuginfo.

These drawbacks are the main reason why restriction 3 "debug-info independence" is added to the motivation.

About platform support
The only significant drawback of relying on debug info for panic messages that I'm aware of is platform support. What I mean is that platforms which don't have a generator and parser/backtracer for debug info would lose panic location information entirely. So how bad is it?

DWARF platforms: AFAIK the entire linux/unix world and even Mac OS's dSYMs are based on DWARF. Today's implementation is rather disappointing from that perspective as it only fully works on Linux (rust-lang/rust#24346) for now. Then there's gimli which implements a DWARF parser in Rust. After toying around with their addr2line implementation for a bit, I was successfully able to read source location info from a symbol-only file for a panic originating in the corresponding stripped version. Perhaps it's not mature enough just yet as some addresses did not work that the binutils addr2line successfully resolved but it still shows that DWARF platforms is very much solvable in its entirety (gimli is fully portable).
PDB platforms (aka Windows): I'm not very famililar with those specifics, perhaps @retep998 can weigh in. All I know is that backtraces are supposed to work so I don't see why this shouldn't.
Emscripten: I honestly have no idea whatsoever.
Anything else? AFAICS these three cover all supported platforms, up to and including tier 3.

I also agree that reliance on debug info is not that bad. But it's great that the RFC comes with a working prototype.
How about going with inline/closure for the initial implementation (as an implementation detail), and switching it to debug-info-based implementation later when backtraces are more reliable in Rust?
I suppose #[implicit_caller_location] is implementable either way. I'm not sure about panic::Location. Perhaps that could be kept unstable?

I think that a more general solution, such as proper default arguments, would be preferable. This proposal is quite a deep cut for such a specific concern. What if we would like to add other implicit arguments later?
I'm also a bit uncomfortable with the fact that the function signature is kind of changed but purported not to be. This is either weird, or a powerful feature that should be generalised.
Perhaps a simple stack trace would be better together with a tool like addr2line. Only the developer needs to know about file names and line numbers. They need not be shipped anywhere.

I'd think const generics https://github.com/rust-lang/rfcs/pull/2000 could be leveraged to provide implicit arguments.

I would much prefer if information about file names and line numbers was not part of the binary itself. If it wants to get the information from the debug info, sure, but don't compile it in to the binary itself please.

@main-- I guess we agree to disagree than. Anyway,

Removed the point about inline-inaccuracy and embedding environment. BTW,

Yes inlined info works on Linux.
Windows 8 claimed to support it but I can't check. I only have tested on Win7 + VS2010, the backtraces do have line info, but the inlined functions are missing.
Can't verify on macOS, needs rust-lang/rust#43422 to land first (not sure why it is idle for 8 days). There is also rust-lang/rust#43635 for Redox, again untouched for 5 days 😕.
Not sure about Android and BSD.

Expanded rationale why release mode caller info is useful. I think whether the debuginfo is inside or outside the binary is irrelevant to the discussion. I also think how to use debuginfo to symbolize error report is off-topic.

@pornel

Changing to rely in debuginfo in the future is certainly possible (provided all the drawbacks fixed), but I'd suggest we at least have an FCP if we do change it. I'm not keen on having libstd depending on gimli and having to read an external file just to get the location, but we already depend on libbacktrace, so #whatevah.
std::panic::Location is already stable. You mean Location::caller()?

@burdges That breaks object-safety
@retep998 Before this RFC every time you call panic!, unreachable! and indexing a slice, a new copy of file/line/column info will be placed into the binary already, so I don't see this as a big issue compared with the status-quo.
Outsourcing the panic location business to *.pdb-reading may warrant a separate issue itself...

I think the major benefit of this proposal over debuginfo is that it's a language guaranteed system instead of a tooling and platform system.

I would much prefer if information about file names and line numbers was not part of the binary itself

I think there are cases where you don't want to ship debuginfo, but are okay with shipping panic info. Furthermore, debuginfo is unreliable, e.g. when inlining is involved.

@oli-obk, that can just as well be regarded as a drawback. Why complicate the language and bloat the produced binaries when a simple tool could accomplish the same thing?
Just let the panic handler print a stack trace if compiled as non-release. Pick up the addresses and convert them to source information.
Debug info is not a language concept.

Why complicate the language and bloat the produced binaries when a simple tool could accomplish the same thing?

@repax Because it's can't. Not reliably. Unfortunately, things are not as simple as "just printing a stack trace". I showed that it's possible to somehow make it work but that's far from the ironclad guarantee this RFC could provide.
Platform support is a major drawback of my proposal, that's no secret. (Even though I don't really mind it myself).

I think there are cases where you don't want to ship debuginfo, but are okay with shipping panic info.

@est31 Could you please elaborate on that? A significant part of my reasoning still relies on the assumption that this case is not realistic, so I'd be very interested in some counterexamples.

I think whether the debuginfo is inside or outside the binary is irrelevant to the discussion.

@kennytm I don't - this is specifically what allows us to have our cake and eat it too since you can either distribute the debug symbols separately or, in the case of closed-source software, keep them and use them to decode bug reports.

These signal that debuginfo support is not reliable enough if we want to solve the unwrap/expect issue now.

You're right. My proposal is not able to solve this problem immediately, a lot of work has to be done first.

FWIW we discussed this RFC on our podcast: https://request-for-explanation.github.io/podcast/ep7-unwrapping-a-great-rfc/index.html

I just thought of a cool usecase for this feature that I hope can be supported. If this feature can be used on <MyError as From<OtherError>>::from and the blanket <T as Into<U>>::into where U: From<T>, this can be used to store line information in errors using the try operator.

@jethrogb It can be done with both proposals (it is the "Runtime-free backtrace for ? operator" section), but both PoC don't support it because the transformation were performed before monomorphization.

Yes, thanks, I didn't see that.

@jethrogb

How could this proposal possibly be made to work with trait methods?

Perform the change after monomorphization, which means either perform the change at trans level, which I haven't investigated yet; or introduce post-monomorphization MIR pass, which is a pretty big change to the compiler.

Does this mean you're generating two different versions of the function?

Yes there are two different versions.
fn foo_real(args, location: Location) -> R {   // <-- this is the "closure".
   // real code
}
fn foo(args) -> R {
    foo_real(args, (file!(), line!(), column!()))
}

When using direct call to foo, the compiler can rewrite them direct call to foo_real. But this can't work for function pointers since there will be no place to provide the extra location argument.
This is why some people suggested to introduce an ABI for this, as ABI will allow us to supply the caller location with function pointer as well. But that will break backward-compatibility since the function-pointer type is changed.

Oh, its only now that I've found out that a completely different concept was chosen, not semantic inlining but a mixture of inlining and having a function with additional parameters.
I'm not really a great fan of the new approach.
What are the advantages? I only have found one in the RFC text:

However, typical implementation will still need to manually implement the two functions, otherwise the code will become very bloated after all the inlining.

Not really. Most functions are one liners, a few have more lines, like 3 or so. Most likely LLVM inlines them already.

@est31

Not really. Most functions are one liners, a few have more lines, like 3 or so. Most likely LLVM inlines them already.

See https://github.com/rust-lang/rfcs/pull/2091#issuecomment-319600862 ("inling is painful to debug") and https://github.com/rust-lang/rfcs/pull/2091#issuecomment-319755605 (several reasons against) — looks like people do want to write large functions with this attribute on.

@kennytm could you include them?

@est31 Okay.

I am suddenly wondering... has a solution based on thread-local storage been explored?
There are drawbacks, certainly. Most notably, thread-local storage and coroutines do not mix well (coroutines have to be aware of thread-local storage to make it coroutine-local instead).
The idea is however rather simple:
 thread_local! { static LOCATION: Cell<Option<&'static Location>> = Cell::new(None); }

Then, the answer to what is the location is in a regular function:
true_location!()

And in a ghost function:
LOCATION.get().unwrap()

which should be hidden behind within whatever magic is necessary.
Calling a ghost function from a normal function is:
LOCATION.set(Some(true_location!()));
call_ghost_as_usual();
LOCATION.set(None);

Calling a ghost function from a ghost function is:
call_ghost_as_usual();

Calling a regular function from a ghost function is:
let __previous_location = LOCATION.get().unwrap();
call_regular_as_usual(); // may override LOCATION (or not)
LOCATION.set(Some(__previous_location));

So, the drawbacks first:

coroutines and thread-local storage interaction,
a slight run-time overhead.

There are significant benefits:

the signature of the function is unchanged,
the ABI of the function is unchanged (as the transformation is automatic),
it works without issue with recursive functions.

And there is the elephant in the room:

how to define the interaction with trait methods?

On the latter point, I see two solutions:

a method can only be ghost if the base trait method is ghost, so that the caller always statically knows whether the method may be ghost or not (and if a method is not ghost and the base is, maybe it needs to save/restore LOCATION),
allow whatever combination, and use a LOCATION.get().unwrap_or(true_location!()) if the method is ghost and the base is not (in which case it works in static contexts but not dynamic ones).

Thoughts?
Note: I foresee that the Option<&'static Location> should likely be a *const Location to avoid unwrap reading the location... but the logic seems clearer as such.

@matthieu-m Yes I have though about TLS before but I don't even consider to include them as a "non-viable alternative" since (1) I don't think the overhead of TLS is ignorable (2) TLS is not available everywhere, and it doesn't really provide the benefits you've mentioned:

the signature of the function is unchanged

With #[implicit_caller_location] the signature of the outer function is unchanged either.

the ABI of the function is unchanged (as the transformation is automatic),

but how do you know to update the TLS LOCATION without an ABI or type-state?
let f: fn(Option<u32>) -> u32;
if random() < 0.5 {
    f = Option::unwrap;
} else {
    f = |_| 0;
}
f(None);  // How should we know we need to update LOCATION here?

If we don't care about stability of function pointers, then ABI difference can be ignored as well.

it works without issue with recursive functions.

Neither is  #[implicit_caller_location].

Micro-benchmark of assigning to local variable vs TLS. TLS takes at least 5x longer.
running 3 tests
test test_local ... bench:          40 ns/iter (+/- 5)
test test_tls   ... bench:         459 ns/iter (+/- 124)
test test_tls_2 ... bench:         239 ns/iter (+/- 69)

test result: ok. 0 passed; 0 failed; 0 ignored; 3 measured; 0 filtered out

Source code
#![feature(test, thread_local, const_fn)]

extern crate test;
use test::{Bencher, black_box};
use std::cell::Cell;

thread_local! {
	static FOO: Cell<(&'static str, u32, u32)> = Cell::new(("foo.rs", 1, 2));
}

// #[thread_local] currently requires Sync, see rust-lang/rust#35035.

struct FakeSyncCell<T>(Cell<T>);
unsafe impl<T> Sync for FakeSyncCell<T> {}

#[thread_local]
static BAR: FakeSyncCell<(&'static str, u32, u32)> = FakeSyncCell(Cell::new(("foo.rs", 1, 2)));

#[bench]
fn test_tls(bencher: &mut Bencher) {
	bencher.iter(|| {
		for _ in 1 .. 100 {
			black_box(FOO.with(|x| x.set(("foo.rs", 1, 2))));
		}
	});
}

#[bench]
fn test_local(bencher: &mut Bencher) {
	// It doesn't need to be a Cell but let's be fair.
	let local = Cell::new(("foo.rs", 1, 2));
	bencher.iter(|| {
		for _ in 1 .. 100 {
			black_box(local.set(("foo.rs", 1, 2)));
		}
	});
}

#[bench]
fn test_tls_2(bencher: &mut Bencher) {
        bencher.iter(|| {
                for _ in 1 .. 100 {
                        black_box(BAR.0.set(("foo.rs", 1, 2)));
                }
        });
}

FWIW, I've encountered no fewer than six bugs related to backtrace production and resolution - even addresses are not guaranteed. Having a single - useful - frame that is guaranteed would be incredibly useful, and make Rust 300% less painful in daily life. So a strong :+1: from me for implementing this ASAP.

I find that I haven't addressed @pornel's comment yet.

The fact that it ties code generation to error reporting is unfortunate. To minimize inlining side-effect I'd probably resort to using two functions instead of one.

Actually this is something that the old version of RFC handles, but the new one does not. The problem is to distinguish between "your fault" and "my fault":
use std::collections::HashMap;
use std::hash::Hash;

fn count_slices<T: Hash + Eq>(array: &[T], window: usize) -> HashMap<&[T], usize> {
    if !(0 < window && window <= array.len()) {
        panic!("invalid window size");
        // ^ triggering this panic is "your fault"
    }
    let mut result = HashMap::new();
    for w in array.windows(window) {
        if let Some(r) = result.get_mut(w) {
            *r += 1;
        } else {
            panic!("why??");
            // ^ triggering this panic is "my fault"
            //   (yes this code is wrong and entry API should be used)
        }
    }

    result
}

The old version of the RFC requires explicitly passing caller_location!() to signify "your fault" like:
panic_at_source_location!(caller_location!(), "invalid window size");

The other panic will be "my fault".
In the new version, everything is forwarded automatically, which means every panic inside #[implicit_caller_location] is "your fault". One simple solution is to separate the "my fault" panic and "your fault" panic into two, but since declarative macro 1.0 is insta-stable, it would give a part of the RFC an unnecessary higher barrier of acceptance.
The concept of "your fault" is same as "preconditions" in Design by contract. Rust supported DbC via the hoare plugin in older nightlies, although the preconditions are still panicking at the "my fault" location. (Similarly, D's DbC implementation also asserts at "my fault" location instead of "your fault".)
Swift does distinguish between the two kinds of panics semantically. The "your fault" ones are called precondition, while the "my fault" ones are called assert, though they don't deal with caller location, and practically they are equivalent to Rust's assert! and debug_assert!. This also suggests we can still separate existing panicking macros into the "my fault" and "your fault" camps accordingly:

Definitely "my fault" (use actual location): debug_assert! and friends, unreachable!, unimplemented!
Probably "your fault" (propagate caller location): assert! and friends, panic!

The question is, should calling unwrap(), expect() and x[y] (index()) be "my fault" or "your fault"? Let's consider existing implementation of index() methods:
// Vec::index
fn index(&self, index: usize) -> &T {
    &(**self)[index]
}

// BTreeMap::index
fn index(&self, key: &Q) -> &V {
    self.get(key).expect("no entry found for key")
}

// Wtf8::index
fn index(&self, range: ops::RangeFrom<usize>) -> &Wtf8 {
    // is_code_point_boundary checks that the index is in [0, .len()]
    if is_code_point_boundary(self, range.start) {
        unsafe { slice_unchecked(self, range.start, self.len()) }
    } else {
        slice_error_fail(self, range.start, self.len())
    }
}

If they all get #[implicit_caller_location], the x[y], expect() and slice_error_fail() should all report "your fault", i.e. caller location should be propagated downstream. It does mean that the current default of caller-location-propagation-by-default is more common when the code is finalized. This also means "my fault" happening during development may become harder to spot. This can be solved using

RUST_BACKTRACE=1.

Bring back the ***_at_source_location functions and macros so user could decide which location to report.

Restrict to debug_assert!/unreachable! for "my fault" stuff

Provide the caller location as a "semantic-inline-static-stack-trace", so the programmer can see every function involved and determine whose fault is it themselves. Note this would make code interacting with #[implicit_caller_location] consume even more resource.

Split into two functions, as @pornel has resorted to:

use std::collections::HashMap;
use std::hash::Hash;

#[implicit_caller_location]
fn count_slices<T: Hash + Eq>(array: &[T], window: usize) -> HashMap<&[T], usize> {
    if !(0 < window && window <= array.len()) {
        panic!("invalid window size");  // <-- your fault
    }
    (|| {
        let mut result = HashMap::new();
        for w in array.windows(window) {
            if let Some(r) = result.get_mut(w) {
                *r += 1;
            } else {
                panic!("why??"); // <-- my fault (caller propagation can't go into closures)
            }
        }
        result
    })()
}

Anyway, treating everything as "your fault" will encourage that #[implicit_caller_location] functions should be short, which goes in line with the "must have" list in the RFC. Thus I think it is fine.

Thank you for addressing this. It's a shame that macros are hard to get to stable, since precondition!() would be great.
Could this RFC be designed to make custom precondition!() macro possible? (e.g. instead of looking for #[implicit_caller_location] on the function, look  for something in the function, like a call to caller location intrinsic or attr inside the function).

For me I wrote an unwrap!() macro for results and an always!() macro for Option<> that printed the line numbers on panic. What I can't do is generate a stack trace with line numbers - this is key to rust adoption because it's got to be so much less work than perfecting the debuggers on the three main platforms. I can live without line numbers or a decent debugger, but not both - I'm really looking forward to progress on this front!
As an aside, #[implicit_caller_location] reminds me of C#'s [DebuggerStepThrough] attribute when you want to signify to the debugger this isn't a call that one would usually want to debug into. I wonder whether one implies the other...

What about nested calls of functions with this attribute? I.e., what happens when a #[implicit_caller_location] function calls another #[implicit_caller_location] function? Is the caller location properly propagated down two levels? I think this is supposed to be the case, but was not able to find an explicit statement along these lines in the RFC.

@RalfJung Yes the location will be propagated downwards, though it is implied indirectly in https://github.com/kennytm/rfcs/blob/inline-semantic/text/0000-inline-semantic.md#redirection-mir-inlining.

I think we definitely need a way to distinguish “my fault” from “your fault”.  At the very least, it would make it much easier to determine who to report a bug to: a precondition failure is a bug in the caller, while an internal assertion failure is a bug in the callee.

OK, I skimmed the new draft. I definitely prefer the newer description -- in particular, I like to think of inlining as an implementation detail. If needed, we could implement it with some implicit parameters (e.g., a const char * and an unsigned int, in C terms) that we thread down as extra arguments to such a call.
I am relatively convinced that it would be nice to make this available -- if we can do so in a very light-weight fashion, and perhaps in a kind of "best-effort" capacity -- even when debuginfo is unavailable. That said, I think that, as debuginfo improves, it will likely want to take these annotations into account (moreover, I would think that if you opt to compile with debuginfo, we might make it an option to have Location::new() use debuginfo as one possible implementation strategy).
So at the highest level we are saying:

add Location::new(), which gives you the "current location" where an error should be reported
functions tagged with #[implicit_caller_location] cause Location::new to return the caller's location (or at least make a best effort to do so)

I think we should change the names. I think the word "blame" is maybe useful here. e.g., maybe something like BlameLocation and #[blame_caller]? I like #[blame_caller], at least, and I think that Location seems too generic (though I might prefer SourceLocation::at_fault() or something).
On other thing: I think we should limit this to inherent methods and free functions -- in other words, non-trait-based dispatch. Thus I would prohibit such annotations from traits and trait impls, at least for now. This still covers unwrap(), expect(), and most other cases; it does not cover error-chain.

@nikomatsakis Thanks for the comment :)

I think we should change the names. I think the word "blame" is maybe useful here. e.g., maybe something like BlameLocation and #[blame_caller]? I like #[blame_caller], at least, and I think that Location seems too generic (though I might prefer SourceLocation::at_fault() or something).

I'm fine with any names. I'm choosing Location since we're reusing std::panic::Location, and is compatible with #2070 which suggests to move those types into core::. Change it to BlameLocation or SourceLocation means the core:: name and std:: name will be different (which currently causes documentation problem rust-lang/rust#42675, though the issue is not a concern here).

I really like this RFC. In particular, I'd love this, less for panicking, but more for the error_chain-like aspect of it.
That said, I do see the value for panicking, but I would be very careful about adding #[implicit_caller_location] to indexing. I suspect a lot of code relies on "panic-fusion" (as described by the RFC in the optimisation section) to be fast when implementing traditional, index-based algorithms. .get(i).unwrap(...) when the debug info would be crucial.

As long as this feature can be completely disabled so that no cost or bloat is incurred then I don't care. This is only useful for debugging.
A true stack-based introspection would be technically more interesting - connecting caller addresses to const symbols (crate, module, fn)...

Updated RFC to include the discussion in https://github.com/rust-lang/rfcs/pull/2091#issuecomment-322003974 and suggestion from https://github.com/rust-lang/rfcs/pull/2091#issuecomment-326377279.

Renamed #[implicit_caller_location] to #[blame_caller].
Removed the section about the Try trait since we are not going to apply the attribute to traits.
Specified only panic!() and assert{,_eq,_ne}!() will receive caller locations.
Included DbC as an alternative.

Some names are kept unchanged:

The internal attribute remained #[rustc_implicit_caller_location], as I don't see a reason to change it
The location lang item remained at core::panicking::Location, so it is compatible with #2070.
The method name is still Location::caller() at the moment, as the PoC implementation already has a Location::new(&str, u32, u32) 😄

Super-nit: since we had to rename bad_style, are we going to have to rename this to praise_caller?

In the alternatives section, the Design-by-contract solution looks great to me, except the syntax (writing code inside attributes feels wrong).
Would it be possible to implement another syntax for it? Perhaps as a #[precondition] attribute on a block or expression?
fn foo(x: i32) {
   #[precondition] {
       asset!(x > 0);  // caller location
   }
}

It was previously mentioned that macros are insta-stable, but that's just an implementation issue? Can the feature flag implementation be improved, or the process changed? e.g. support precondition!() macro without a feature flag, but only on nightly until it's ready to go stable.

I think design by contract should be implemented in a library (or multiple libraries, so the ecosystem can work out the best syntax/subfeatures for it). It should definitely use the feature proposed in this RFC, but this feature is very useful even in non-DbC contexts such as today's std, so it shouldn't be tied specifically to DbC.

Since we're rapidly approaching the impl period, and this RFC appears to be headed toward rough consensus, I'm going to go ahead and kick off the review process for FCP, to make sure this gets in front of the lang team.
@rfcbot fcp merge

@nikomatsakis

I think we should limit this to inherent methods and free functions -- in other words, non-trait-based dispatch. Thus I would prohibit such annotations from traits and trait impls, at least for now. This still covers unwrap(), expect(), and most other cases; it does not cover error-chain.

Can you elaborate on your reasoning for this? I think being able to use this for Error types would be a huge deal, maybe even more important than its use for panics. It seems to me that it would be a waste if we'd go through all the trouble of implementing this feature but then not use it a lot of the time. As mentioned, debug info can already help you a lot of the time when dealing with panics. Debug info can't help you when dealing with Error types (unless you'd inspect the stack every time you're generating an error value, which seems like a bad idea).

I think that preconditions are a better solution. They would be injected at the call site and their assertions would be evaluated there.
If the compiler has/gets enough smarts then some of these preconditions could be determined at compile time. This means both simple conditions such as arg1 > 5 and more complex ones that make use of CTFE or const fn #911.

@repax
There are two problems of preconditions:

If unwrap() uses #[precondition], it would need to perform two is_some() check, once in the precondition and once in the main body. We can rely on the optimizer to get rid of the second check.

More generally, I haven't thought of how "injecting the precondition at call site" will interact with function pointers:
#[precondition(x <= 0)]
fn negative_sqrt(x: f32) -> f32 { (-x).sqrt() }

#[precondition(x >= 0)]
fn positive_sqrt(x: f32) -> f32 { x.sqrt() }

fn abs_sqrt(x: f32) -> f32 {
    let f: fn(f32) -> f32;
    if x >= 0 {
        f = positive_sqrt;   // are we going to allow this?
    } else {
        f = negative_sqrt;
    }
    f(x)
}

I expect both functions cannot be cast to a function pointer, but that means stabilized APIs Option::unwrap may not be able to use the #[precondition] approach.

BTW the precondition can simply be injected as normal assert!() expressions. Static analysis tool, or even the compiler itself, could try to deeply const-fold the function body, and issue a warning/error if it turns out to be unconditionally diverging.

Anyway, when this RFC is implemented, #[precondition] can first be implemented an attribute macro built on top of #[rustc_implicit_caller_location] (not #[blame_caller]).
#[precondition(condition, "fmt_args", a, b, c, d)]
fn foo(a: A, b: B, c: C, d: D) -> E {
    bar()
}

// would become

#[rustc_implicit_caller_location]
fn foo(a: A, b: B, c: C, d: D) -> E {
    (move |__location| {
        assert!(condition, "fmt_args", a, b, c, d);
        (move || {
            bar()
        })()
    })(caller_location())
}

If unwrap() uses #[precondition], it would need to perform two is_some() check, once in the precondition and once in the main body. We can rely on the optimizer to get rid of the second check.

The check in the precondition will be instantiated (and possibly optimised away if statically determined). The one in the main body can simply be omitted (via unsafe) because it is redundant.

More generally, I haven't thought of how "injecting the precondition at call site" will interact with function pointers:

Pointers to functions with preconditions will have to point to closures. You'll get the source location of where the pointer was assigned, which is pretty good. The compiler may choose to inline the fn, as usual.
This RFC solves a subset of what preconditions can do. Preconditions are a valuable feature for safety critical systems, so language support for this is something that should be seriously considered (see Ada for example).

@pornel

Or as a precondition!() macro. It was previously mentioned that macros are insta-stable, but that's just an implementation issue, right? Can the feature flag implementation be improved, or the process changed? e.g. support precondition!() macro without a feature flag, but only on nightly until it's ready to go stable.

Initially I tried using a syntax similar to D:
fn positive_sqrt(x: f32) -> f32
requires! { //~ ERROR unexpected token
    assert!(x >= 0, "cannot accept negative numbers");
}
{
    x.sqrt()
}

but turns out a macro cannot be placed there. Since I'm not really seriously try to introduce DbC into the language, I just reused hoare's syntax 😄.
And again, a procedural macro can transform any precondition syntax into the form in https://github.com/rust-lang/rfcs/pull/2091#issuecomment-327439149, so this can be experimented in the crates.io system first.

@repax

Pointers to functions with preconditions will have to point to closures. You'll get the source location of where the pointer was assigned, which is pretty good.

This leads back to the issue raised in the RFC.
let f: fn(Option<u32>) -> u32 = Option::unwrap;
let g: fn(Option<u32>) -> u32 = Option::unwrap;
assert!(f == g); // This must remain `true`.
f(None);
g(None); // The effect of these two calls must be the same.

This RFC solves a subset of what preconditions can do.

Disagreed, preconditions cannot be used to implement Location::caller().

I agree that precondition is a viable alternative, but I think this should be proposed as a separate RFC (after the impl Period).

@kennytm: Right, so this must not be allowed:
#[precondition(x <= 0)]
fn negative_sqrt(x: f32) -> f32 { (-x).sqrt() }
...
let f: fn(f32) -> f32 = negative_sqrt;

for the reason is that the precondition is erased by type: fn(f32) -> f32. Thus, the preconditions must be referenced by the signature. Intriguing...

You mention, in the RFC, the -Z location-detail flag. When releasing software you must choose to either include or not include source information. What do you think of the possibility of supplying a uniquely generated number for each call site? The developer could then match these to the actual location without having to include any other source information in the binary.

@repax

What do you think of the possibility of supplying a uniquely generated number for each call site?

Yes it is possible, but reading the backtrace would provide same information? That is, the "unique number" is the address of the callsite, and the developer uses the debug-info to decode the address into source information.

Yes, using a stack trace is what I initially proposed but was told it couldn't reliably be done. A generated number is the next best thing for released software, or disabling the feature altogether.

@repax It is not reliable for reporting the file/line/column info alongside the panic message. What you want  (decode the number in another place) is different from this. Anyway this has been discussed before.
A generated unique number can be produced using the hash value of the call site, written to the line/column value of the Location, but the compiler will need to save the mapping information into some files for developer's reference. The implementation effort is not trivial.
Meanwhile, you could also just disable the feature with -Z location-detail=none, generate the debuginfo with -g/-C debuginfo=2, optionally use https://github.com/rust-lang/rfcs/pull/2091#issuecomment-319781144 to separate out the debuginfo.
I think, for your purpose, you don't need to give up the "best thing".

Could the RFC state that on nested calls of #[implicit_caller_location]-annotated functions the error location is properly propagated to the caller? It is currently hard to extract this information from the RFC.

unless you'd inspect the stack every time you're generating an error value, which seems like a bad idea

@jethrogb Is it really that bad? Error paths aren't supposed to be hot anyways. And even then, just creating a stack trace is cheap - as far as I remember it's quite literally a standard linked list (at least on x86) so performance is not an issue. You would only run the slow parts (resolving addresses to names) when actually printing the error, but it's totally fine if that part is slow (if you print an error, it's definitely not a hot path).

Error paths aren't supposed to be hot anyways.

Why not? Errors are cheap in Rust. String parsing and various try methods on sync primitives all return errors that are not necessarily cold paths.

@jethrogb

Can you elaborate on your reasoning for this? I think being able to use this for Error types would be a huge deal, maybe even more important than its use for panics

So, actually, I think what I said doesn't make much sense. =) I wanted to preserve the ability for this to be implemented by 'extra arguments' that are silently inserted as part of the ABI. I realize now that -- whether we implement this through "extra monomorphization" or "extra parameters" -- the important thing is static vs dynamic dispatch.
So long as the final dispatch is static, then at code generation time (i.e., after monomorphization), we know whether to thread the "extra location" argument through, whether the call was encoded via a trait or not.
Once we get into dynamic dispatch -- either through a coercion to a fn ptr or through a trait object -- then things get problematic. I think it's a non-starter to add extra arguments ubiquitously, because of the runtime cost, and monomorphization isn't even possible.
So, at that point, I think we get back to the point I made about this being best effort. That is, at least during dynamic dispatch, you have to expect that your location will not be the caller, but rather the current line, even if the fn is tagged as #[blame_caller].
Best effort isn't really the best term: I think we should define what you will get, but just say that when dynamic dispatch is involved, you don't get the caller's location but your own. (But with static dispatch, you would always get the location in your caller (or caller's caller, if they are blaming their caller too).

Error paths aren't supposed to be hot anyways.

Why not? Errors are cheap in Rust. String parsing and various try methods on sync primitives all return errors that are not necessarily cold paths.

@jethrogb That's a different kind of error though. I emphatically agree that Result is great even for very high performance APIs like AtomicUsize but you would never directly propagate the result of a compare_exchange or try_lock with error-chain. You fully expect these calls to fail (in the case of try_lock you're calling the function specifically because it can fail) so you always handle the error right there. I consider these kinds of errors orthogonal to this discussion.
My point here is that - as I already mentioned - just generating a stack trace is negligibly cheap. They only cases where it may have any significant performance impact at all are high-performance APIs like compare_exchange or try_lock but they don't matter here because you wouldn't generate stack traces for them anyways.

I think the new blame_caller name coupled with Location is not ideal because the names don't suggest that they have anything to do with each other. I'd like the attribute to mention location in some way, like

#[keep_caller_location]
#[store_caller_location]
#[caller_location]
#[propagate_location]

Or something.

@rfcbot concern explicit-doc-nested-calls-with-blame-caller
When I first started reading the Guide level docs, I had incorrectly started off thinking that the only effect of #[blame_caller] was going to be to change the source locations emitted by instances of assert! and panic! etc that occur in this function body to blame my immediate caller.
Indeed, from reading the comment thread, it appears that @RalfJung previously raised the question of what happens when you nest calls to such functions, and @kennytm responded by pointing out the answer, which is implied by the redirection implementation.)
So I failed to infer two important details when I read the Guide:

assert!/panic!/etc in my body do not necessarily blame my immediate caller. If my caller is tagged with #[blame_caller], then the blame will propagate to its caller, and so on, until we have an untagged function somewhere along the chain.
likewise, my being tagged with #[blame_caller] might change the source locations for assert! and panic! invocations for functions that I call, so that they now blame my caller as well.

I say "might" because it depends on whether all intermediate functions that are called are all tagged with #[blame_caller], of course.

Note: I am not saying that the latter is a bad semantics!
Point 2 is a logical consequence of point 1. But I point them out separately, because I think the two points-of-view have different trade offs:

Point 1 makes sense from the view point of "As the callee, I want to know whom to blame for this ill-formed input; if my immediate callers are all delegating responsbility, then yes, I want to skip over them until someone says 'the buck stops here'."
Point 2 seems problematic. If we have a call-chain f -> g -> h, I can easily picture a scenario where we want assertions in the body of fn g to be blamed on the call-site within fn f, but we want assertions in the body of fn h to be blamed on the call-site within fn g.

The RFC does not explicitly document how to handle this f -> g -> h scenario, though it does discuss the My Fault/Your Fault distinction, which is related, and actually the work-around shown there should also apply here.
Namely, a workaround could be to split out code into closures, and then have the call chains end up looking like f -> g -> closure1 -> h, f -> g -> closure2 -> h

Another way to address Point 2 could be by adding a #[blame_here] attribute, where statements/expressions with that attribute will feed in their own location rather than the location passed by the caller.

This also seems like a reasonable way to also address the My Fault/Your Fault distinction, that would probably be more readable than factoring code into closures:

use std::collections::HashMap;
use std::hash::Hash;

#[blame_caller]
fn count_slices<T: Hash + Eq>(array: &[T], window: usize) -> HashMap<&[T], usize> {
    if !(0 < window && window <= array.len()) {
        panic!("invalid window size");  // <-- your fault
    }
    let mut result = HashMap::new();
    for w in array.windows(window) {
        #[blame_here]
        if let Some(r) = result.get_mut(w) {
            *r += 1;
        } else {
            panic!("why??"); // <-- my fault
        }
    }

    result
}

Anyway, this concern is mostly me trying to ensure that this detail about the semantics of nested calls ends up being spelled out in the guide-level documentation.

@rfcbot concern which-abis-does-this-work-with
Does the #[blame_caller] attribute work with any fn regardless of its ABI? Or is it only meant to be used with extern "rust" fn?

Does the #[blame_caller] attribute work with any fn regardless of its ABI? Or is it only meant to be used with extern "rust" fn?

Very good point. I feel strongly it should be only rust functions. =)

I'm going to check off my rfcbot check-box because I do think we should move forward on this RFC and I don't think the concerned I raised should be considered blocking.
But at the same time, I really would like them addressed in the document before we merge it.
@kennytm do you think you'll have a chance to do so?

@pnkfelix Sorry for long delay. I've addressed parts of your comments in 3ddca9d:

Explicitly mentioned the caller location will be propagated in the Guide-Level Description.
Mentioned #[blame_caller] will not work with #[naked] or extern "ABI" (except extern "Rust" of course) because you can't apply these to a closure. These will become hard errors in the final implementation. Still, I think this is just a technical restriction. Semantically, I think all attributes and ABIs should be able to mix with  #[blame_caller].

I did not include #[blame_here], because AFAIK the attributed blocks will be stripped out before  MIR translation, e.g.
#![feature(custom_attribute)]

fn main() {
    let mut u = 1;
    #[blame_here] {   // <-- MIR won't recognize this block has an attribute
        u = 2;
    }
    u = 3;
}

To support #[blame_here], the HIR → MIR translation and MIR structures will need to be change to retain the attributed blocks or store the attributes in each TerminatorKind::Call. While this makes MIR much more flexible, using it just for #[blame_here] seems a waste of effort.

@rfcbot resolved explicit-doc-nested-calls-with-blame-caller

@rfcbot resolved which-abis-does-this-work-with

I like this idea a lot. However I do not feel that only printing the blamed caller is sufficient. If a user is unfamiliar with the function marked #[blame_caller] it may not know which panic message it emits and will  look for the panic! call in the caller. If a call to #[blame_caller] is indirect, the panic message will show as being in the callee, while if the call is direct, the message will show as being in the caller. This is confusing. Another question which was raised is if the caller is really to blame. I suggest we let #[blame_caller] mean blame the caller too, and print both the caller and callee. @kennytm suggested that  earlier and I think this is by far the best approach. This would mean chains of direct #[blame_caller] calls would print a stack trace.
If you marked all functions as #[blame_caller] we could effectively get stack traces for Rust functions. Amusingly the current RFC wording would blame fn main(), which is technically correct, but not very helpful. I personally find stack traces invaluable and I propose we have a code generation option which lets Rust build a stack trace at runtime. It should change the "Rust" calling convention to also make indirect calls work. This would mean Rust require no platform support for perfect stack traces if you stick to using just functions with the "Rust" calling convention. This is particularly important given that Rust currently doesn't have this ability on any platform. LLVM without optimizations enabled gets pretty close, but that has a massive runtime performance hit. This would also be ridiculously helpful for bare metal or embedded work.

Regarding size and runtime overhead. The part about having more static struct Location objects (size overhead) is clear, but can the panic branch still be #[cold] after this change?

@Zoxc I'm afraid passing a whole stack trace would make the "overhead" disadvantage even worse 😄 Also, I'm interested in how you're going to record the stack trace in bare metal, as making the location the blame_location a stack will need a singly-linked list or dynamic array, which can't work directly in no_std in either case.
@arthurprs Rust can't mark a branch as #[cold] yet (we could use std::intrinsics::likely but this currently can't be applied on match). The panic_fmt() function remains #[cold] #[inline(never)].

@kennytm I think I wasn't clear, by panic branch I meant what is currently fn expect_failed(msg: &str) -> !.
I guess it could become fn expect_failed(msg: &str, loc: &Location) -> !, right?

@arthurprs Oh. Yes it could become fn expect_failed(msg: &str, loc: &Location) -> !, but that's pointless compared to directly calling core::panicking::panic (also #[cold] #[inline(never)]) as they have similar signatures.

Indeed, with the addition of location it becomes meaningless.

@retep998 Before this RFC every time you call panic!, unreachable! and indexing a slice, a new copy of file/line/column info will be placed into the binary already, so I don't see this as a big issue compared with the status-quo.

I'm not a fan of the status quo either in this case. I'd rather not have any such information forced into the binary and require debuginfo to be used, at least for platforms like windows-msvc where debuginfo is in external files and backtraces are reliable.

@kennytm I don't buy the overhead argument. Having a stack trace would be 3 additional instructions per call on x86 which isn't very significant. Also #[blame_caller] should mostly be applied to simple functions which get inlined. In that case the runtime overhead would be only for the panic case, which makes it insignificant. If you really care about performance (or code size), you are looking for an option to disable all panic messages as @retep998 suggests.
You can build a linked list on the stack by having a node on the stack link to some node in an ancestor stack frame. This is how structured exception handling (SEH) is done in Windows on x86-32. You can even do it in safe Rust:
struct Location<'a> {
    func: &'static str,
    parent: Option<&'a Location<'a>>,
}

fn print_stack_trace<'a>(loc: Location<'a>) {
    let mut current = Some(&loc);
    while let Some(loc) = current {
        println!("{}", loc.func);
        current = loc.parent;
    }
}

fn foo<'a>(loc: Location<'a>) {
    print_stack_trace(Location {
        func: "foo",
        parent: Some(&loc),
    })
}

fn main() {
    foo(Location {
        func: "main",
        parent: None,
    });
}

@retep998 Isn't it just 16 bytes per entry? Maybe it can default to line to lower the cost even more (12?).

I'm not fully sold on the importance of debug-info independence.
On platforms without debug info support there are usually other issues, like: binary size concerns, no unwinding, etc..

debuginfo doesn't. work.
On Sep 27, 2017 3:52 AM, "Arthur Silva"<EMAIL_ADDRESS>wrote:

I'm not fully sold on the importance of debug-info independence.
On platforms without debug info support there are usually other issues,
like: binary size concerns, no unwinding, etc..
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/rust-lang/rfcs/pull/2091#issuecomment-332470562, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAGln2o9vhWXoYfeqTMVQbZBJJNWc_Sgks5smhrigaJpZM4OopRf
.

When I first saw this RFC (or rather its predecessor, semantic inlining), I really hadn't spent a lot of time thinking about the entire topic of panic locations in Rust. But even though the "you're unwrapping None somewhere good luck guessing where" message has certainly annoyed me in the past, I immediately felt that this is not the right approach to this issue.
After some discussion here and writing up my counter-proposal at #2154, I think I can narrow it down to a core issue: I don't think this RFC is solving the right problem. I disagree entirely with two of its design goals:

Finding a single function to blame is hard - often, bugs are rather caused by multiple components interacting in unexpected ways. The basic motivaton for both this and #2154 is that panic messages are useless when they don't print the right location, so printing a different location and hoping that it's the right one this time doesn't look like a solid and clean solution.
Unstrippable panic messages even in release builds are a drawback, not a feature in my opinion.

Sorry for adding to an already-long discussion, but I'm going to anyway. The solution proposed in this RFC may very well be the best solution to the to the unwrap error messages problem, especially given the amount of discussion that has gone on and the current consensus to merge. And I don't deny that the #[blame_caller] syntax looks elegant and is obvious in what it does, and is probably a useful feature that will prove to be useful for lots of other things besides unwrap.
I just want to point out how great it would have been if unwrap had instead been a macro that was allowed in postfix/method position, e.g. result.unwrap!(). Then we would have had error messages with file names and line:column numbers from the beginning. In fact, when I first started learning Rust, I somehow got it into my head that method-style macros were a thing, and was surprised to learn that unwrap was just an ordinary method.
The unwrap! macro is actually not that complicated to implement, by the way. Here's a gist and a playpen link that I put together today. You can do a lot of cool stuff by combining macros and traits!

Firstly, let me echo the sentiment that the RFC is very well-written.
Unfortunately, it seems like a lot of machinery for little gain. I'm supremely worried about the performance impact. Nowadays we have crates reimplementing unwrap to make it friendlier; tomorrow we will have crates reimplementing unwrap to make it faster. Unless this behavior applies only to debug builds, we will be no better off. Worse off, perhaps, because of all the new language machinery.
In addition, I'm not convinced that we ought to be making unwrap friendlier in the first place. Unwrap is something that you occasionally must do, even though you don't want to, which is why its performance still matters. But it would be counterproductive to encourage its use any further.

I couldn't read all the discussion, so I'm sorry if it was already said.
This functionality seems also useful eg. for logging when a given function wants to be "invisible" and report/use location of a caller, not itself.
Because of that, I wonder if blame is not too narrow and specific. Just because it was introduced for the given purpose, doesn't mean it should be named like that. #[caller_location] or something would explain what it does, and not what it was introduced for.

caller_location doesn't have a verb, so it sounds odd IMHO, as if the attribute was saying "this is the caller location" (which of course it isn't). #[use_caller_location]? The previous #[implicit_caller_location] wasn't too bad, but #[blame_caller] is shorter and easier to spell :)

@pornel #[track_callers] Sounds good to me!

I think that the language feature requested in this RFC is both too invasive and too specific. I feel like it's a kludge that, if accepted, will haunt the Rust spec for eternity.
I'd rather see a general solution that allows you to include code in the caller's context that surround the called function (AOP-style but inline), or more work on guaranteeing not just the type of an argument but also its value.
Being able to guarantee values statically (or semi-statically) at the type level is a lot more general and powerful, and something that we would eventually like to tackle. Think of Ada-like value ranges or an effect system encoded in the types of objects and functions.
I'd like to see this proposal postponed until evaluated against long-term goals of the Rust language -- i.e. evolved static guarantees and so on.


Don't print nested stack traces for run() failures

In python3, stack traces seem to report if the raised exception happened
during another exception handling. It will then say something like
"exception happned during handling of above exception". This is nice,
but just cluttery in the case of our tracking_url_callback hack.

Does this look sane to you @erikbern?


[URGENT][BUG] Status of users are "blinking" and changing without any change on back-end

@rafaelks Is this at toolbar or sidebar?

@leonardoaramaki Sent information in private, but basically on left-sidebar.


[MPWG- design task] MicroProfile Member Banner

use the mktg drive design for drafts
gDrive

@aeiras @jclingan @dblevins
I started to work on the member and compatible badges for MP. After our marketing call yesterday the majority of people want to see a badge that is simple. So based off this I came up with first version. ( I put both here so you can see them next to each other)

@rstjames looks awesome.
Given that there are 2 tickets, can you delete the screen shot that is not associated with the task on this ticket and the other?

@rstjames please drop the draft design in the folder.
The ticket has been move to the MPWG repo. https://github.com/microprofile/microprofile-wg/issues/39


Accessing vertices positions from specific body regions?

Hi @ keijiro
I was wondering how could I access the skinned mesh vertices using your project.
I would like to know how can I access a list of vertices so then I can select random vertices from that list. Then I would like to grow a plant or flowers at random locations.
Is this possible with your setup or would it be overkill?
Thank you for your time and for sharing all your beautiful work?
Guillermo

This project has been outdated. I'd recommend using VFX Graph instead. This kind of customization is easier on it compared to Skinner. See this example for details: https://github.com/keijiro/Smrvfx


more effective test for parallel traversals

Scope & Purpose
As we can't check query execution plans for traversal parallelization, this now adds a failure point that is only triggered during parallel execution. That way the tests have an unambiguous way to tell if traversal parallelism was applied or not.
Community part of https://github.com/arangodb/enterprise/pull/604
Backport of https://github.com/arangodb/arangodb/pull/13220

[x] :hankey: Bugfix (requires CHANGELOG entry)
[ ] :pizza: New feature (requires CHANGELOG entry, feature documentation and release notes)
[ ] :fire: Performance improvement
[ ] :hammer: Refactoring/simplification
[ ] :book: CHANGELOG entry made

Backports:

[x] No backports required

Related Information

[x] Enterprise PR: https://github.com/arangodb/enterprise/pull/604

Testing & Verification

[x] This change is a trivial rework / code cleanup without any test coverage.
[x] The behavior in this PR was manually tested

Link to Jenkins PR run:
http://<IP_ADDRESS>:8080/view/PR/job/arangodb-matrix-pr/13215

Tests blue except known, unrelated issue

Tests blue except unrelated failure in test (which was fixed in separate commit).

http://jenkins01.arangodb.biz:8080/view/PR/job/arangodb-matrix-pr/13234/

Tests blue.


Fix problems in PR2117

This PR would fix some problems in PR#2117.

It would solve the AppVeyor compile error. Probably it is a bug in the VS compiler.

As https://github.com/nlohmann/json/pull/2117#issuecomment-634475426 said, I found some problems.

2.a. I used these code to detect but just found JSON_HAS_CPP_17 = False in all travis jobs.
#ifdef JSON_HAS_CPP_17
    std::cout << "JSON_HAS_CPP_17 = True" << std::endl;
#else
	std::cout << "JSON_HAS_CPP_17 = Flase" << std::endl;
#endif

Thus, All travis jobs didn't run the testcase - TEST_CASE("std::optional"), and those all code about optional were not compiled.
I add a new travis task for c++17.
2.b. I also found the actual behavior, which would also happen in linux:
CHECK(std::optional<std::string>(j_string) == opt_string);  // call from_json(const BasicJsonType& j, ConstructibleStringType& s)
CHECK(std::optional<bool>(j_bool) == opt_bool); // call from_json(const BasicJsonType& j, typename BasicJsonType::boolean_t& b)
CHECK(std::optional<int>(j_number) == opt_int); // call from_json(const BasicJsonType& j, ArithmeticType& val)

And the testcase SECTION("null") didn't call the expected method either and raise an excepiton - "[json.exception.type_error.302] type must be object, but is null".
I agree with this view:

The reason is the template optional(U&&) constructor template, which is supposed to activate when T (int) is constructible from U and U is neither std::in_place_t nor optional, and direct-initialize T from it. And so it does, stamping out optional(foo&).

Thus, they would not call the method from_json(basi_json, std::option<T>) never. This is why an exception is raised in SECTION("null").
Related Problem:
Different results in Clang and GCC when casting to std::optional
Why is a cast operator to std::optional ignored?
specification of std::optional constructor regarding cast operators

Coverage remained the same at 100.0% when pulling e0f6c96c194c97ff9eb749979216c2d4da173067 on dota17:fix-C2440 into b61d563ebe23b9e9ddc860d8abf7af4b01979aa4 on nlohmann:feature/optional.


Tripple file upload notification for Android

Describe the bug
When uploading a photo I get three identical upload notifications
To Reproduce
Steps to reproduce the behavior:

Open Textile Photos
Create a group
Upload picture to the group

Expected behavior
A single notification
Smartphone (please complete the following information):

Device: Pixel 2XL
OS: Android
Version Q (beta)

Additional context
I actually don't want any notifications that my files were successfully uploaded. How can I turn this off?

Hey thanks for opening an issue. When you share a photo in Textile Photos, there are actually three uploads that happen: small, medium, and high resolution files. We're using a library for uploading the files that, by default, displays a notification for each upload. We're currently in the process of writing our own custom uploading code that will handle notifications is a better way. You can track progress here https://github.com/textileio/android-textile/issues/17