Add context for more "strict undefined behavior" cases #627
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
fasterthanlime
commented
Oct 31, 2024
fasterthanlime
force-pushed
the
keep-ctx-undefined-behavior
branch
from
0d428c9 to
a7b0df9
Compare
mitsuhiko
requested changes
Oct 31, 2024
minijinja/src/vm/mod.rs
Outdated
| @@ -306,7 +318,7 @@ impl<'env> Vm<'env> { | |||
| Instruction::EmitRaw(val) => { | |||
| // this only produces a format error, no need to attach | |||
| // location information. | |||
| ok!(out.write_str(val).map_err(Error::from)); | |||
| ctx_ok!(out.write_str(val).map_err(Error::from)); | |||
There was a problem hiding this comment.
This case is intentionally bypassing ctx_ok! as you can see from the comment. This is done for performance reasons.
minijinja/src/vm/mod.rs
Outdated
| @@ -279,6 +279,18 @@ impl<'env> Vm<'env> { | |||
| }; | |||
| } | |||
|
|
|||
| #[allow(unused_macros)] | |||
There was a problem hiding this comment.
While I think this is a nice idea, at least the raw case should be allowed to use the ok! macro as mentioned. I would probably just not do the ok! macro here. Particularly with the fixes to ? this codebase will avoid the ok! and some! macros in the future anyways.
...and make it hard to forget to add context again
fasterthanlime
force-pushed
the
keep-ctx-undefined-behavior
branch
from
a7b0df9 to
3b9ff86
Compare
|
@mitsuhiko Thanks for the review! I removed the ok-shadowing and let EmitRaw use it — it's now just 3 occurences of ok=>ctx_ok |
fasterthanlime
added a commit
to axodotdev/cargo-dist
that referenced
this pull request
Oct 31, 2024
For now, this depends on a branch of minijinja, until <mitsuhiko/minijinja#627> lands
fasterthanlime
added a commit
to axodotdev/cargo-dist
that referenced
this pull request
Oct 31, 2024
For now, this depends on a branch of minijinja, until <mitsuhiko/minijinja#627> lands
fasterthanlime
added a commit
to axodotdev/cargo-dist
that referenced
this pull request
Nov 1, 2024
For now, this depends on a branch of minijinja, until <mitsuhiko/minijinja#627> lands
fasterthanlime
added a commit
to axodotdev/cargo-dist
that referenced
this pull request
Nov 1, 2024
For now, this depends on a branch of minijinja, until <mitsuhiko/minijinja#627> lands
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
...and make it hard to forget to add context again.
This hit us in https://github.com/axodotdev/cargo-dist when trying to enable strict undefined behavior.
The change I do care about is using
ctx_ok!in all branches of the vm evaluation.I added a little trick where the
ok!macro is shadowed, so if you accidentally use it again, rustc will yell at you:Other ideas include.. shadowing
ok!in the first place and changing everyctx_ok!invocation back took!, but eh, I had to pick one. Feel free to pick your favorite / take over this PR / do whatever it takes to get this merged, all I care is that our errors have debug info :) Thanks!