Implement RFC 2011 (nicer assert messages) #48973

sinkuu · 2018-03-13T03:23:21Z

assert!(x + y == z && w);

thread 'main' panicked at 'assertion failed: x + y == z && w
with expansion: 3 + 5 == 7 && (unevaluated)', test.rs:6:5

Detailed designs

🚲 🏠 What should be captured? (Current implementation captures operands of unary and binary operators.) How should the panic message look like?

Performance

Now assert expands to some amount of code, and this may slow down compilation and execution. We need to test performance difference and see if it have to fall back to the original behavior when not(debug_assertions).

oli-obk · 2018-03-13T11:32:32Z

Current implementation captures operands of unary and binary operators.

I think function call arguments could be captured, too. There's no difference between Add::add(&a, &b) and a + b in my book.

pietroalbini · 2018-03-13T12:12:45Z

Highfive failed to assign a reviewer, assigning one randomly from the libs team.

r? @dtolnay

bors · 2018-03-14T04:28:10Z

☔ The latest upstream changes (presumably #48684) made this pull request unmergeable. Please resolve the merge conflicts.

sinkuu · 2018-03-18T00:33:22Z

I think function call arguments could be captured, too. There's no difference between Add::add(&a, &b) and a + b in my book.

I thought so too, but realized that they are different after trying to implement.

let x = &mut Foo;
let y = &mut Foo;

let _ = bar(x); // x is reborrowed
println!("{:?}", x);

let _ = x + y; // x is consumed
// println!("{:?}", x); // "use of moved value: `x`"

This is problematic because my strategy to capture an intermediate value by-value is to replace a expression with

{ let tmp = {expression}; try_copy(&tmp, &mut __capture); tmp }

which always consumes the expression.
I tried applying another transformation like { try_copy(&{path}, &mut __capture); func({path}) } for ExprKind::Path (where reborrow works?) to preserve reborrow. This seemed to work at first, but bumped into func(None) (None is a Path on AST) causing type inference error. 😑

shepmaster · 2018-03-23T15:36:17Z

Ping from triage, @dtolnay ! Will you have time to review this in the near future?

bors · 2018-03-23T21:49:33Z

☔ The latest upstream changes (presumably #49308) made this pull request unmergeable. Please resolve the merge conflicts.

dtolnay

I still need to look at the implementation of Context::generate but here are some comments regarding the rest.

dtolnay · 2018-03-25T12:34:38Z

src/libcore/lib.rs

+    #[unstable(feature = "generic_assert_internals", issue = "44838")]
+    impl<T> Debug for DebugFallback<T> {
+        default fn fmt(&self, f: &mut Formatter) -> fmt::Result {
+            f.write_str(self.alt)


Is it possible for self.value to be Unevaluated here? If so, I would expect this to print "(unevaluated)" even for non-Debug types.

Right, this needs to be

match self.value { Value | NotCopy => alt, Unevaluated => "(unevaluated)", }

dtolnay · 2018-03-25T12:39:44Z

src/libsyntax_ext/assert.rs

@@ -86,7 +761,7 @@ fn escape_format_string(s: &str) -> String {

 #[test]
 fn test_escape_format_string() {
-    assert!(escape_format_string(r"foo{}\") == r"foo{{}}\\");
+    assert!(escape_format_string("foo{}") == "foo{{}}");


Is this change necessary? The implementation of escape_format_string has not changed.

dtolnay · 2018-03-25T12:41:01Z

src/libsyntax_ext/assert.rs

+    /// True if `||` or `&&` are contained in this tree.
+    contains_lazy: bool,
+    /// True if by-value captures are contained in this tree.
+    contains_by_ref: bool,


The field name contradicts the doc comment (by-ref vs by-value).

dtolnay · 2018-03-25T12:56:37Z

To help review, would you be able to share what src/libsyntax_ext/assert_tests.rs looks like after macro expansion? rustc -Z unstable-options --pretty=expanded

sinkuu · 2018-03-25T14:48:38Z

https://gist.github.com/sinkuu/ba6abd95cb55e839ae33cb022410bb36
(I added dummy macro_rules! panic { ($($t:tt)*) => {panic($($t)*)}; } to prevent explosion with format! expansion)

ishitatsuyuki · 2018-03-29T09:47:03Z

@dtolnay I guess that your review comments are addressed. Mind doing another review?

dtolnay · 2018-03-29T09:55:55Z

src/libcore/lib.rs

@@ -208,3 +208,87 @@ pub use coresimd::simd;
 #[unstable(feature = "stdsimd", issue = "48556")]
 #[cfg(not(stage0))]
 pub use coresimd::arch;
+
+// FIXME: move to appropriate location


Please move this to libcore/asserting.rs (by analogy with runtime support for panicking in libcore/panicking.rs).

dtolnay · 2018-03-29T09:59:54Z

Thanks! I would be comfortable accepting this if for now we make it expand exactly the old way in release mode. Later we can debate what changes it makes sense to apply in release mode.

ishitatsuyuki · 2018-03-30T08:15:03Z

Is this insta-stable? I have no strong opinion but maybe it's good to have a feature flag to switch between new and old behaviour.

Regarding the performance concern, does it make sense to have a perf run (while we have this enabled as default)?

sinkuu · 2018-03-30T22:33:51Z

#49071 (comment) applies to this PR, too:( I believe "plain text fallback" described in RFC2011 cannot be implmented without this specialization (T -> T: Debug).

ishitatsuyuki · 2018-04-05T02:44:19Z

This RFC can be backward compatible only if the broader specialization is used; do you think we can use that here? Is there a soundness issue? cc @nikomatsakis

Back to the implementation itself, can we track the evaluated status by using a counter? Short circuits has a defined ordering, and presumably computing that at codegen time allows better optimization.

sinkuu · 2018-04-05T03:16:48Z

I'm not sure whether if I get what you mean. Maybe listing all evaluation states and have branches of panic call for each of them? I'd avoid that as it would make explosion of the amount of the generated code much more worse. I think optimization for the case of assertion falure is not important and should not regress other factors.

ishitatsuyuki · 2018-04-05T03:33:11Z

Sounds like you're right. Never mind then.

bors · 2018-04-06T12:10:50Z

☔ The latest upstream changes (presumably #49154) made this pull request unmergeable. Please resolve the merge conflicts.

nikomatsakis · 2018-04-09T18:37:09Z

@ishitatsuyuki

Is there a soundness issue? ... Sounds like you're right. Never mind then.

I'm not sure if my input is still needed, can you clarify? =)

ishitatsuyuki · 2018-04-09T22:46:39Z

@nikomatsakis As we are using a variant of specialization that isn't going to be stabilized, we need your input on whether it's acceptable to use such specialization or not. Also see #49071 which tries to achieve a similar specialization.

sinkuu · 2018-04-15T23:08:15Z

I will reopen once the discussion about sound specialization settles.

sinkuu force-pushed the nicer_assert branch 2 times, most recently from 92bf267 to 1a2577d Compare March 13, 2018 09:36

pietroalbini added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 13, 2018

pietroalbini assigned dtolnay Mar 13, 2018

sinkuu force-pushed the nicer_assert branch 3 times, most recently from b386e12 to 63b84e8 Compare March 19, 2018 02:34

Implement RFC 2011

aa6d303

sinkuu force-pushed the nicer_assert branch from 63b84e8 to aa6d303 Compare March 24, 2018 05:58

sinkuu added 3 commits March 25, 2018 10:11

Remove unnecessary Rc

9b8e000

Refactor

8aa55e7

Add comment

f07097a

sinkuu force-pushed the nicer_assert branch from 6375457 to f07097a Compare March 25, 2018 03:47

dtolnay reviewed Mar 25, 2018

View reviewed changes

sinkuu added 2 commits March 25, 2018 23:44

Fix comment

259498d

Revert unnecessary change

521a510

Fix DebugFallback default impl

9aa8cac

dtolnay reviewed Mar 29, 2018

View reviewed changes

sinkuu closed this Apr 15, 2018

c410-f3r mentioned this pull request Apr 28, 2022

Implement RFC-2011 (Nicer assert! messages) #96496

Closed

fmease added the F-generic_assert `#![feature(generic_assert)]` label Apr 8, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement RFC 2011 (nicer assert messages) #48973

Implement RFC 2011 (nicer assert messages) #48973

sinkuu commented Mar 13, 2018 •

edited

Loading

oli-obk commented Mar 13, 2018

pietroalbini commented Mar 13, 2018

bors commented Mar 14, 2018

sinkuu commented Mar 18, 2018

shepmaster commented Mar 23, 2018

bors commented Mar 23, 2018

dtolnay left a comment

dtolnay Mar 25, 2018

sinkuu Mar 25, 2018

dtolnay Mar 25, 2018

dtolnay Mar 25, 2018

dtolnay commented Mar 25, 2018

sinkuu commented Mar 25, 2018

ishitatsuyuki commented Mar 29, 2018

dtolnay Mar 29, 2018

dtolnay commented Mar 29, 2018

ishitatsuyuki commented Mar 30, 2018

sinkuu commented Mar 30, 2018

ishitatsuyuki commented Apr 5, 2018

sinkuu commented Apr 5, 2018

ishitatsuyuki commented Apr 5, 2018

bors commented Apr 6, 2018

nikomatsakis commented Apr 9, 2018

ishitatsuyuki commented Apr 9, 2018

sinkuu commented Apr 15, 2018

Implement RFC 2011 (nicer assert messages) #48973

Implement RFC 2011 (nicer assert messages) #48973

Conversation

sinkuu commented Mar 13, 2018 • edited Loading

Detailed designs

Performance

oli-obk commented Mar 13, 2018

pietroalbini commented Mar 13, 2018

bors commented Mar 14, 2018

sinkuu commented Mar 18, 2018

shepmaster commented Mar 23, 2018

bors commented Mar 23, 2018

dtolnay left a comment

Choose a reason for hiding this comment

dtolnay Mar 25, 2018

Choose a reason for hiding this comment

sinkuu Mar 25, 2018

Choose a reason for hiding this comment

dtolnay Mar 25, 2018

Choose a reason for hiding this comment

dtolnay Mar 25, 2018

Choose a reason for hiding this comment

dtolnay commented Mar 25, 2018

sinkuu commented Mar 25, 2018

ishitatsuyuki commented Mar 29, 2018

dtolnay Mar 29, 2018

Choose a reason for hiding this comment

dtolnay commented Mar 29, 2018

ishitatsuyuki commented Mar 30, 2018

sinkuu commented Mar 30, 2018

ishitatsuyuki commented Apr 5, 2018

sinkuu commented Apr 5, 2018

ishitatsuyuki commented Apr 5, 2018

bors commented Apr 6, 2018

nikomatsakis commented Apr 9, 2018

ishitatsuyuki commented Apr 9, 2018

sinkuu commented Apr 15, 2018

sinkuu commented Mar 13, 2018 •

edited

Loading