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

Fix Rust attributes and doc comments on the first line #957

Merged
merged 5 commits into from
Jul 18, 2019

Conversation

djrenren
Copy link
Contributor

A bug was causing files starting with attributes and doc comments
to produce incorrect error nodes. This change also allows attributes
to occur in positions other than the start of a line, bringing it in
line with the language's grammar.

I still question the inclusion of doc comments because they're not part of the grammar of the language. They're used inside comments containing code to hide irrelevant lines of code while generating documentation. Still, someone probably found them useful so best to keep them working.

@djrenren djrenren changed the title Fix attributes and doc comments on the first line Fix Rust attributes and doc comments on the first line Jul 17, 2018
@dblessing
Copy link
Collaborator

@djrenren Thanks for working on this. Can you help me understand a bit the intention? Are you trying to highlight some specific things in comments (the doc components) as something other than just the comment token?

@djrenren
Copy link
Contributor Author

djrenren commented Aug 7, 2018

So to be clear, everything that follows was already implemented and I simply had to work around it in order to fix attributes:

In rust you can write things called doc comments using triple slashes:

/// Does a thing
fn foo() {}

and these are used to generate HTML docs with a tool called rustdoc. In rouge, these are just regular comments. All is well.

But you can also include code snippets in your doc comments like so:

/// Example:
/// ```
/// foo(2);
/// ```
fn foo(x: i32) -> (){}

And this will put a code example in your generated docs. These are still regular comments as far as rouge is concerned which is good.

There's an accompanying tool to rustdoc that will execute your code snippets in your comments to ensure that your examples are still valid. It's called doctest. This means that sometimes we need to write setup code for our example so that it runs, but we don't want to display that to the end user. You do that like so:

/// Example:
/// ```
/// # let a = 4;
/// foo(a);
/// ```
fn foo(x: i32) -> (){}

That # sign means "execute this line while running doctest, but don't render it to the doc". This is where things get tricky. Someone built in support for these "doc comments" so that if you pass the snippet in the comment to rouge, we'll treat the leading # as a comment. Note that this is not valid rust, it's a peculiarity of a specific (albeit ubiquitous) tool. Comments in rust are delimited with C-style // and /* .. */.

My suspicion is that someone wanted to use rouge to highlight snippets from rustdoc which included these doc comments, so they decided to make # a valid indicator for comments (but only at the start of the line).

Anyway, the code that supported that was tied up with the code for attributes (because they both start with #) and was annoying to untangle. I left it in because it was beyond the scope of the change, but I'm on the fence about whether it should stay in the highlighter.

@pyrmont pyrmont added the needs-review The PR needs to be reviewed label Jul 9, 2019
djrenren and others added 5 commits July 19, 2019 03:27
A bug was causing files starting with attributes and doc comments
to produce incorrect error nodes. This change also allows attributes
to occur in positions other than the start of a line, bringing it in
line with the language's grammar.
@pyrmont
Copy link
Contributor

pyrmont commented Jul 18, 2019

@djrenren I have two apologies:

  1. I'm sorry this took so long to get addressed. I'm afraid that I've been going through the backlog of PRs in reverse chronological order and it took a while to get to this one.

  2. I'm sorry to overwrite your branch with a force push. Your solution worked great but I wanted to do a couple of things to get it ready for merging:

    1. To reduce the warnings generated by Rouge when Ruby is run with warnings enabled, we've moved to avoid ambiguous regular expression literals. There are multiple ways to do this; we generally take the simple approach of changing rule /.../ to /rule %r/.../ (which is what I did here).

    2. Your solution of using an instance variable worked but the idiomatic way in Rouge to do what you were doing is to use a :bol state that is pushed onto the stack after each newline and to make it the starting state by adding push :bol to the block passed to the start method.

Thanks for going to the effort of fixing the problem while maintaining the existing behaviour (which, I agree, is probably not really something the Rust lexer should support). It's really appreciated and good to have this finally in Rouge :)

@pyrmont pyrmont merged commit e3598c6 into rouge-ruby:master Jul 18, 2019
@pyrmont pyrmont removed the needs-review The PR needs to be reviewed label Jul 18, 2019
@djrenren
Copy link
Contributor Author

djrenren commented Jul 19, 2019 via email

@pyrmont
Copy link
Contributor

pyrmont commented Jul 19, 2019

Thanks! We've moved to a two-week release cadence while we clear the backlog so version 3.7.0 of the gem will be published on RubyGems next Tuesday :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants