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

Words on the first line being dropped? #76

Closed
jmackie opened this issue Oct 1, 2021 · 9 comments · Fixed by #81
Closed

Words on the first line being dropped? #76

jmackie opened this issue Oct 1, 2021 · 9 comments · Fixed by #81
Labels
bug Something isn't working good first issue Good for newcomers Hacktoberfest Stuff that's good for hacktoberfest contribs help wanted Extra attention is needed

Comments

@jmackie
Copy link

jmackie commented Oct 1, 2021

Hey,

First up, this library is dope. Love your work. 🚀

I'm having one small issue though - if I have a SourceSpan that points somewhere in the first line of some source code, that first line seems to get chopped.

Here's a test to reproduce:

#[cfg(test)]
mod miette_test {
    use miette::{
        Diagnostic, GraphicalReportHandler, GraphicalTheme, SourceSpan, ThemeCharacters,
        ThemeStyles,
    };
    use thiserror::Error;

    #[derive(Error, Debug, Diagnostic)]
    #[error("some error")]
    #[diagnostic(help("where did the word 'imagine' go!?"))]
    struct SomeError {
        #[source_code]
        some_code: String,
        #[label("'imagine' should come before this")]
        some_span: SourceSpan,
    }

    #[test]
    fn whytho() {
        let mut rendered = String::new();
        let diagnostic = SomeError {
            some_code: "imagine this is some code\n\nyou're seeing all of this line".to_owned(),
            some_span: (8, 0).into(),
        };

        GraphicalReportHandler::new()
            .with_theme(GraphicalTheme {
                characters: ThemeCharacters::ascii(),
                styles: ThemeStyles::none(),
            })
            .with_context_lines(3)
            .render_report(&mut rendered, &diagnostic)
            .unwrap();
        assert_eq!(
            &rendered,
            r#"
  x some error
   ,-[1:1]
 1 | this is some code
   : ^
   : `-- 'imagine' should come before this
 2 | 
 3 | you're seeing all of this line
   `----
  help: where did the word 'imagine' go!?
"#
        );
    }
}
@zkat
Copy link
Owner

zkat commented Oct 1, 2021

That's very strange. I literally have tests for this: https://github.com/zkat/miette/blob/main/src/source_impls.rs#L190-L271

Can you tell what corner case there you happen to be running into? Sorry this happened!

@jmackie
Copy link
Author

jmackie commented Oct 1, 2021

I have no idea what's going on, but to be clear I was expecting the first line of that report to render as...

 1 | imagine this is some code

Maybe I should have posted a failing test rather than trying to explain my expectation in the actual test 🤦

@zkat
Copy link
Owner

zkat commented Oct 1, 2021

This is also what I would expect!

@zkat zkat added bug Something isn't working good first issue Good for newcomers help wanted Extra attention is needed Hacktoberfest Stuff that's good for hacktoberfest contribs labels Oct 1, 2021
@towaanu
Copy link
Contributor

towaanu commented Oct 2, 2021

Hello, I'll be happy to help with this issue !

The issue occurs every time there is a break line ( \n ) and a SourceSpan on the first line.
The problem comes from the context_info function in source_impls.
When constructing MietteSpanContents an offset is adding to data ( I am not sure what data property should contain in MietteSpanContents ) . If no offset is added ( &input[0..offset] ) everything is displayed however offset is not taking into account ( (4,2) gives the same result as (0, 2) ) when there is a breakline and a SourceSpan on the first line .

@zkat
Copy link
Owner

zkat commented Oct 2, 2021

I think you've almost got it.

I suspect this is the specific bit that's breaking, because there's no before_context_lines in this particular case.

My guess is that the fix will either involve flipping that conditional (oops), or just adding another condition in place.

To explain a bit what data is: data is the entire span, including context lines, that is supposed to be highlighted. The current tests are all correct in that context, so I'd like to keep them that way.

@zkat
Copy link
Owner

zkat commented Oct 2, 2021

anyway, PRs welcome if you make a test that minimally reproduces this, and then fix that :)

@zkat
Copy link
Owner

zkat commented Oct 2, 2021

Here's a failing test:

+
+    #[test]
+    fn multiline_with_context_line_start() -> Result<(), MietteError> {
+        let src = String::from("aaa\nxxx\n\nfoo\nbar\nbaz\n\nyyy\nbbb\n");
+        let contents = src.read_span(&(2, 0).into(), 2, 2)?;
+        assert_eq!(
+            "aaa\nxxx\n\n",
+            std::str::from_utf8(contents.data()).unwrap()
+        );
+        assert_eq!(0, contents.line());
+        assert_eq!(0, contents.column());
+        let span: SourceSpan = (0, 9).into();
+        assert_eq!(&span, contents.span());
+        Ok(())
+    }

@zkat
Copy link
Owner

zkat commented Oct 2, 2021

It occurred to me after my last comments that I need to add some context: The whole reason for this weirdness is that I tried to solve the problem that #20 is supposed to generalize: Snippets are meant to be windows into the SourceCode that we're trying to render. Good enough windows to show the user what's wrong.

But we only have so much space to do it!

So this code tries to do two things at once:

  1. If the text is one long line (for example, a single line with thousands of columns of raw JSON), it "skips forward" to where the span is and renders a window into it.
  2. If the text is multiple lines, it renders a number of context lines before and after.

If both try to happen at the same time, or if our checks run into those corner cases, everything breaks, and that's what's actually happening here.

So... the Ideal Solution is actually to just solve #20 and add a "bounding box width" option that will give us the width, alongside the height (context lines), for our little bounding box. That's an API breaking change, though, so I'd rather just fix this issue's bug individually if we can, for now. I just thought the context might help make sense of why this code is being so weird.

@towaanu
Copy link
Contributor

towaanu commented Oct 2, 2021

Thank you for all these helpful information :) It's very clear, what data is and how is it used !
Since we don't have a "bounding box width" option yet, I assume that, for now, if there is no context lines before ( context_lines_before is 0 ), the line is "skipped forward" to the span. Otherwise, lines are rendered from the start.
I'll try to make a PR for it !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working good first issue Good for newcomers Hacktoberfest Stuff that's good for hacktoberfest contribs help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants