Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use correct line offsets for doctests #47274

Merged
merged 2 commits into from Jan 14, 2018
Merged

Conversation

Manishearth
Copy link
Member

Not yet tested.

This doesn't handle char positions. It could if I collected a map of char offsets and lines, but this is a bit more work and requires hooking into the parser much more (unsure if it's possible).

r? @QuietMisdreavus

(fixes #45868)

@Manishearth
Copy link
Member Author

this solution is currently incorrect, ignore it for now

@Manishearth Manishearth force-pushed the rustdoc-span branch 2 times, most recently from c90c134 to fe33a42 Compare January 8, 2018 16:14
@kennytm kennytm added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-dev-tools-rustdoc labels Jan 8, 2018
@Manishearth
Copy link
Member Author

Ready for review. r? @QuietMisdreavus

@Manishearth
Copy link
Member Author

Need to write a test, unsure where it should go. maketest perhaps?

}

// Now push any outer attributes from the example, assuming they
// are intended to be crate attributes.
prog.push_str(&crate_attrs);
line_offset += crate_attrs.lines().count();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is line_offset supposed to be the number of lines inserted by rustdoc? IIRC, crate_attrs is actually the #![] attributes from the example itself.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. Given that the attrs can only be up top we should just not count this.

@QuietMisdreavus
Copy link
Member

Nice! If i'm looking at this correctly, this will help map between the line that rustc reports an error at, and the line in the actual doctest that was responsible for it? Do we already map these lines back to the file it's in? I'm thinking about this in terms of getting the lines to work with doc(include). That doesn't have to be this PR - it looks like this was an issue before that came in.

Re: where/how to test this, i'd take a look at whether/where the regular line number reporting is tested, and seeing if you can piggyback onto that by making it call rustdoc --test instead.

@Manishearth
Copy link
Member Author

this will help map between the line that rustc reports an error at, and the line in the actual doctest that was responsible for it?

yeah.

Do we already map these lines back to the file it's in?

We say "foobar.rs - itemname (line N)". But then the rustc error shows <stdin>:line:col which is wrong.

@Manishearth
Copy link
Member Author

Added test. The existing stuff isn't easy to rejigger; it's built around assuming rustc. I just wrote a simple run-make test.

@QuietMisdreavus
Copy link
Member

Seems good to me. Let's get this in!

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2018

📌 Commit 44b659a has been approved by QuietMisdreavus

@QuietMisdreavus QuietMisdreavus added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 12, 2018
@bors
Copy link
Contributor

bors commented Jan 14, 2018

⌛ Testing commit 44b659a with merge 5d6f6e6...

bors added a commit that referenced this pull request Jan 14, 2018
Use correct line offsets for doctests

Not yet tested.

This doesn't handle char positions. It could if I collected a map of char offsets and lines, but this is a bit more work and requires hooking into the parser much more (unsure if it's possible).

r? @QuietMisdreavus

(fixes #45868)
@bors
Copy link
Contributor

bors commented Jan 14, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 5d6f6e6 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: very unhelpful span info
4 participants