improve error reporting if outer delimiters are available#194
Merged
sunshowers merged 4 commits intoJun 11, 2024
Merged
Conversation
Created using spr 1.3.6-beta.1
Created using spr 1.3.6-beta.1
5 tasks
ahl
reviewed
Jun 11, 2024
ahl
left a comment
Collaborator
There was a problem hiding this comment.
- Nice find!
- Do we want to update the main lib.rs info and/or README? In particular to guide users who may encounter a similar situation as you did.
- This may be somewhere in the dumb-to-optimistic range, but given the input
TokenStreamcould we construct aSpanthat encompasses its entirety and use that by default? - ... Or was this just the wrong interface and we should have had users pass in the outer, braced tokenstream??
Collaborator
There was a problem hiding this comment.
let's delete this file? I probably shouldn't have added it in the first place. I bet we no longer need the nightly rustfmt?
Comment on lines
+67
to
+68
| /// This is useful with nested attributes inside annotations, where | ||
| /// `serde_tokenstream` is used to parse an attribute inside a top-level macro. |
Collaborator
There was a problem hiding this comment.
Suggested change
| /// This is useful with nested attributes inside annotations, where | |
| /// `serde_tokenstream` is used to parse an attribute inside a top-level macro. | |
| /// This is useful with nested attributes inside annotations, when | |
| /// parsing an attribute nested inside an outer macro. |
maybe?
| /// | ||
| /// ```rust,ignore | ||
| /// #[derive(Record)] | ||
| /// #[record { worker = "Homer J. Simpson", floor = 7, region = "G" }] |
Collaborator
There was a problem hiding this comment.
Created using spr 1.3.6-beta.1
Contributor
Author
Done.
We could, but that would cause an inconsistency where only the part inside the braces would be included, not the braces themselves. I think the current approach is probably fine -- it complains about the whole macro.
Ah sadly the outer TokenStream isn't available with top-level attributes. |
Created using spr 1.3.6-beta.1
ahl
approved these changes
Jun 11, 2024
Collaborator
|
In v0.2.1 now on crates.io |
sunshowers
added a commit
to oxidecomputer/dropshot
that referenced
this pull request
Jun 12, 2024
This is a bit of a basic/elementary test but I realized we didn't have coverage for it. (And it's actually important for server traits -- specifically, without oxidecomputer/serde_tokenstream#194, incorrect span information is produced.)
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Currently, if there's a top-level error in the macro (e.g. a top-level missing
field), then we always get a call_site error. That is because we don't have the
span information corresponding to the outer braces.
This isn't a huge problem if serde_tokenstream is used for the top-level
attribute, but does become one for nested attributes. Trait-based Dropshot
servers are one such case --
endpointannotations are nested under thetop-level and can be pretty far from the call site.
Address this by providing an alternative API that also accepts delimiter spans.
Also remove the
.vscode/settings.jsonthat's no longer required,and allow the
targetgitignore rule to match a symlink (all my targetdirectories are symlinks to a non-btrfs-snapshotted store).