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

Detect diff markers in the parser #106242

Merged
merged 4 commits into from
Dec 29, 2022
Merged

Detect diff markers in the parser #106242

merged 4 commits into from
Dec 29, 2022

Conversation

estebank
Copy link
Contributor

Partly address #32059.

@rustbot
Copy link
Collaborator

rustbot commented Dec 29, 2022

r? @wesleywiser

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Dec 29, 2022
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

❤️❤️❤️

r=me unless you want a review from someone on t-compiler :)

compiler/rustc_parse/src/parser/item.rs Outdated Show resolved Hide resolved
@estebank estebank force-pushed the diff-markers branch 3 times, most recently from 969697e to 52eb5ea Compare December 29, 2022 03:47
@estebank
Copy link
Contributor Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Dec 29, 2022

📌 Commit 62c8e31 has been approved by jyn514

It is now in the queue for this repository.

@bors bors 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-review Status: Awaiting review from the assignee but also interested parties. labels Dec 29, 2022
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Dec 29, 2022
Detect diff markers in the parser

Partly address rust-lang#32059.
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 29, 2022
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#106208 (Make trait/impl `where` clause mismatch on region error a bit more actionable)
 - rust-lang#106216 (Powershell: Use `WaitForExit` instead of `-Wait`)
 - rust-lang#106217 (rustdoc: remove unnecessary `.tooltip::after { text-align: center }`)
 - rust-lang#106218 (Migrate css var scraped examples)
 - rust-lang#106221 (Rename `Rptr` to `Ref` in AST and HIR)
 - rust-lang#106223 (On unsized locals with explicit types suggest `&`)
 - rust-lang#106225 (Remove CraftSpider from review rotation)
 - rust-lang#106229 (update Miri)
 - rust-lang#106242 (Detect diff markers in the parser)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 031a214 into rust-lang:master Dec 29, 2022
@rustbot rustbot added this to the 1.68.0 milestone Dec 29, 2022
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
Detect diff markers in the parser

Partly address rust-lang#32059.
Aaron1011 pushed a commit to Aaron1011/rust that referenced this pull request Jan 6, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#106208 (Make trait/impl `where` clause mismatch on region error a bit more actionable)
 - rust-lang#106216 (Powershell: Use `WaitForExit` instead of `-Wait`)
 - rust-lang#106217 (rustdoc: remove unnecessary `.tooltip::after { text-align: center }`)
 - rust-lang#106218 (Migrate css var scraped examples)
 - rust-lang#106221 (Rename `Rptr` to `Ref` in AST and HIR)
 - rust-lang#106223 (On unsized locals with explicit types suggest `&`)
 - rust-lang#106225 (Remove CraftSpider from review rotation)
 - rust-lang#106229 (update Miri)
 - rust-lang#106242 (Detect diff markers in the parser)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@rben01
Copy link

rben01 commented Jul 18, 2023

I'd love this functionality but I'm not a huge fan of the error messages themselves.

error: encountered diff marker
  --> $DIR/enum-2.rs:3:1
   |
LL | <<<<<<< HEAD
   | ^^^^^^^ after this is the code before the merge
LL |         x: u8,
LL | |||||||
   | -------
LL |         z: (),
LL | =======
   | -------
LL |         y: i8,
LL | >>>>>>> branch
   | ^^^^^^^ above this are the incoming code changes
   |
   = help: if you're having merge conflicts after pulling new code, the top section is the code you already had and the bottom section is the remote code
   = help: if you're in the middle of a rebase, the top section is the code being rebased onto and the bottom section is the code coming from the current commit being rebased
   = note: for an explanation on these markers from the `git` documentation, visit <https://git-scm.com/book/en/v2/Git-Tools-Advanced-Merging#_checking_out_conflicts>

error: aborting due to previous error

First, I believe these markers are called conflict markers, not diff markers. (IIUC diff markers are e.g., +++ file.rs.)

Second, I think the intermediate markers | and = should also be explained.

Third, I find the help: tips a bit verbose — especially considering that they'll likely line wrap in most terminals — and perhaps redundant given the content of the ^^^^^^^ notes. It might be better to stick the help: info in rustc --explain E#### instead of showing every time this error is encountered. Not sure where I stand on the note: tip; it's also a bit long, and its presence might lead people away from rustc --explain, which is probably undesirable. (The git documentation also has a lot of extraneous info that most users wouldn't need to solve this error; they just need to know which is old and which is new, <<<<<<< or >>>>>>>.)

Finally, I think the words “above” and “after” are a bit ambiguous; there's nearly an entire file above and below the outer conflict markers. (What's important is what's between conflict markers.) Furthermore they aren't quite accurate when there's a ||||||| section, since that section is below the <<<<<<< but not part of “our” code.

I think the following error message would be overall clearer and more accurate:

error: git conflict markers in source code
  --> $DIR/enum-2.rs:3:1
   |
LL | <<<<<<< HEAD
   | ^^^^^^^ between this marker and `|||||||` is the code that we're merging into
   |
LL | ||||||| 4fa0b83
   | ^^^^^^^ between this marker and `=======` is the base code (what the two refs diverged from)
   |
LL | =======
   | ^^^^^^^ between this marker and `>>>>>>>` is the incoming code
   |
LL | >>>>>>> branch
   | ^^^^^^^ this marker concludes the conflict region
   |
   = help: conflict markers indicate that a merge was started but could not be completed due to merge conflicts
   = help: to resolve a conflict, keep only the code you want and then delete the lines containing conflict markers

error: aborting due to previous error

or, if not using diff3,

error: git conflict markers in source code
  --> $DIR/enum-2.rs:3:1
   |
LL | <<<<<<< HEAD
   | ^^^^^^^ between this marker and `=======` is the code that we're merging into
   |
LL | =======
   | ^^^^^^^ between this marker and `>>>>>>>` is the incoming code
   |
LL | >>>>>>> branch
   | ^^^^^^^ this marker concludes the conflict region
   |
   = help: conflict markers indicate that a merge was started but could not be completed due to merge conflicts
   = help: to resolve a conflict, keep only the code you want and then delete the lines containing conflict markers

error: aborting due to previous error

@estebank
Copy link
Contributor Author

@rben01 can you create a ticket with your feedback/reword request?

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. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants