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

Removing duplicate Slice origins and surrounding empty lines #9

Closed
phansch opened this issue Sep 3, 2019 · 4 comments · Fixed by #34
Closed

Removing duplicate Slice origins and surrounding empty lines #9

phansch opened this issue Sep 3, 2019 · 4 comments · Fixed by #34
Assignees

Comments

@phansch
Copy link
Member

phansch commented Sep 3, 2019

I'm currently trying to move the suggestions of the following diagnostic over from Rust to annotate-snippet:

error[E0382]: use of moved value: `x`
  --> /code/rust/src/test/ui/annotate-snippet/suggestion.rs:4:5
   |
 4 |     let x = vec![1];
   |         - move occurs because `x` has type `std::vec::Vec<i32>`, which does not implement the `Copy` trait
 7 |     let y = x;
   |             - value moved here
 9 |     x; //~ ERROR use of moved value
   |     ^ value used here after move

In short, from Rust I can only get a Vec that contains the three lines with the annotations. This Vec contains only the lines with annotations and no other lines around it. Something similar to this:

[
  Line {
      line_index: 4,
      annotations: [
          Annotation {
              start_col: 8,
              end_col: 9,
              label: "move occurs because ...",
          },
      ],
  },
  Line {
      line_index: 7,
      annotations: [
          Annotation {
              start_col: 11,
              end_col: 12,
              label: "value moved here",
          },
      ],
  },
  // etc..
]

I'm currently mapping this to annotate-snippets by turning each Line into a Slice and then calling DisplayList::from(snippet) at the end.

The problem is that each Slice has the origin at the top:

error[E0382]: use of moved value: `x`
  --> /code/rust/src/test/ui/annotate-snippet/suggestion.rs:4:8
   |
 4 |     let x = vec![1];
   |         ^ move occurs because `x` has type `std::vec::Vec<i32>`, which does not implement the `Copy` trait
   |
  ::: /code/rust/src/test/ui/annotate-snippet/suggestion.rs:7:12
   |
 7 |     let y = x;
   |             ^ value moved here
   |
  ::: /code/rust/src/test/ui/annotate-snippet/suggestion.rs:9:4
   |
 9 |     x; //~ ERROR use of moved value
   |     ^ value used here after move

If I set the origin of consecutive slices to None there will still be extra empty padding lines:

error[E0382]: use of moved value: `x`
  --> /code/rust/src/test/ui/annotate-snippet/suggestion.rs:5:8                                                             
   |
LL |     let x = vec![1];
   |         ^ move occurs because `x` has type `std::vec::Vec<i32>`, which does not implement the `Copy` trait
   |
   |
LL |     let y = x;
   |             ^ value moved here
   |
   |
LL |     x; //~ ERROR use of moved value
   |     ^ value used here after move
   |

I'm not sure what the original idea for a Slice was, but it seems like the best approach for those rustc annotations?

I can see three ways to solve this currently:

  1. annotate-snippets doesn't add empty padding lines if the origin is None
  2. annotate-snippets removes the origin and empty padding lines of consecutive Slices that have the same origin. This means annotate-snippet would have to compare the origin of each Slice to origin of the previous Slice.
  3. We implement From<Snippet> for DisplayList by ourselves in rustc.

I would love to hear your thoughts on this before I continue with the annotations =)

cc rust-lang/rust#61809

@zbraniecki
Copy link
Contributor

I'm not sure what the original idea for a Slice was, but it seems like the best approach for those rustc annotations?

The idea behind Slice is that it represents a single, continuous, slice of source that you want to annotate.

In your case, you're trying to something different - you want to fold the lines that don't have annotation in the Slice.

Unfortunately, the data provided by rustc seems to be misaligned with that model, so we'll have to find a way to merge them.

I see two options:

  1. We either treat what you're trying to produce as three Slices, one line each, and add options to display a slice as part of some "multislice" slice, so that it doesn't add leading/trailing empty lines and generally behaves like a "middle" of something else.

  2. Or we find a way to treat the whole piece as a single Slice, and then hide lines without annotations.

I think I prefer the latter, because it feels (<-- bias, intuition, take with a grain of salt) that this is more generic approach that other use cases may want.
If we go this way, we'd need to find a way to produce a single slice out of what rustc produces, and extend our API to allow us to cut out those lines since they don't have annotations.

I assume there's some way to extract the whole slice from the start of the first annotation line to the end of the last annotation line, but if there isn't or you don't want to use it, we can also fake it, and we could have a helper function in rustc bindings that produces sth like:

let x = vec![1];


let y = x;

x; //~ ERROR use of moved value

So, basically fill the missing lines with empty lines. Now, this mocked slice, can be fed to our API with annotations in the right positions.

The last thing we need is some param that would tell the API that if the line has no annotation, skip it.

Would that make sense?

phansch added a commit to phansch/annotate-snippets-rs that referenced this issue Sep 6, 2019
@phansch
Copy link
Member Author

phansch commented Sep 6, 2019

Thanks for your input! I will try to go with option 2 for now and see if I can make it work.

If we go this way, we'd need to find a way to produce a single slice out of what rustc produces

I think that shouldn't be too big of a problem in the end. I was just hoping there was an existing way to work around that (hence the issue) :)

and extend our API to allow us to cut out those lines since they don't have annotations.

I may be missing a piece of the picture here, but isn't that what the fold option of Slice does?

@zbraniecki
Copy link
Contributor

I may be missing a piece of the picture here, but isn't that what the fold option of Slice does?

Yes! I'm just not sure if it does exactly what you need (and what rustc one does). We can fine tune it tho :)

@phansch
Copy link
Member Author

phansch commented Oct 17, 2019

Short update as I'm back working on this. I think I was trying to hard to work with what rustc currently uses for the old emitter.
My new plan is to write my own collect_annotations method that returns data appropriate for annotate-snippet. That should avoid the whole problem from the first post as I'll have full access to the source map.

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 a pull request may close this issue.

2 participants