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

stop using BytePos for computing spans in librustc_parse/parser/mod.rs #68845

Merged
merged 1 commit into from Feb 6, 2020

Conversation

@dwrensha
Copy link
Contributor

dwrensha commented Feb 5, 2020

Computing spans using logic such as self.token.span.lo() + BytePos(1) can cause internal compiler errors like #68730 when non-ascii characters are given as input.

#68735 partially addressed this problem, but only for one case. Moreover, its usage of next_point() does not actually align with what bump_with() expects. For example, given the token >>=, we should pass the span consisting of the final two characters >=, but next_point() advances the span beyond the end of the =.

This pull request instead computes the start of the new span by doing start_point(self.token.span).hi(). This matches self.token.span.lo() + BytePos(1) in the common case where the characters are ascii, and it gracefully handles multibyte characters.

Fixes #68783.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 5, 2020

r? @estebank

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

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 5, 2020

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-02-05T03:34:37.1546778Z ========================== Starting Command Output ===========================
2020-02-05T03:34:37.1548680Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/d7449a3e-d8fd-4114-a028-1835ceda1138.sh
2020-02-05T03:34:37.1548732Z 
2020-02-05T03:34:37.1554582Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-05T03:34:37.1562220Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68845/merge to s
2020-02-05T03:34:37.1564245Z Task         : Get sources
2020-02-05T03:34:37.1564283Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-05T03:34:37.1564321Z Version      : 1.0.0
2020-02-05T03:34:37.1564410Z Author       : Microsoft
---
2020-02-05T03:34:38.2311427Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-05T03:34:38.2323772Z ##[command]git config gc.auto 0
2020-02-05T03:34:38.2326553Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-05T03:34:38.2328882Z ##[command]git config --get-all http.proxy
2020-02-05T03:34:38.2335678Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68845/merge:refs/remotes/pull/68845/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@dwrensha dwrensha force-pushed the dwrensha:fix-68783 branch from bf8b8a8 to 9ac68e1 Feb 5, 2020
@dwrensha

This comment has been minimized.

Copy link
Contributor Author

dwrensha commented Feb 5, 2020

(Repushed after an ./x.py fmt.)

@dwrensha

This comment has been minimized.

Copy link
Contributor Author

dwrensha commented Feb 5, 2020

The diff in the test stderr is:


---- [ui] ui/parser/issue-68730.rs stdout ----
diff of stderr:

33         |        ^
34
35      error: expected one of `>`, `const`, identifier, or lifetime, found `<`
-         --> $DIR/issue-68730.rs:5:11
+         --> $DIR/issue-68730.rs:5:10
37         |
38      LL | enumem˂˂
-          |         ^ expected one of `>`, `const`, identifier, or lifetime
+          |        ^ expected one of `>`, `const`, identifier, or lifetime
40
41      error: aborting due to 5 previous errors
42

}
token::Ge => {
let span = self.token.span.with_lo(self.token.span.lo() + BytePos(1));
Some(self.bump_with(token::Eq, span))
let start_point = self.sess.source_map().start_point(self.token.span);

This comment has been minimized.

Copy link
@estebank

estebank Feb 5, 2020

Contributor

If this is such a common thing then we might want to have a new method in SourceMap for it.

@estebank

This comment has been minimized.

Copy link
Contributor

estebank commented Feb 5, 2020

@bors r+ rollup

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 5, 2020

📌 Commit 9ac68e1 has been approved by estebank

Dylan-DPC added a commit to Dylan-DPC/rust that referenced this pull request Feb 6, 2020
stop using BytePos for computing spans in librustc_parse/parser/mod.rs

Computing spans using logic such as `self.token.span.lo() + BytePos(1)` can cause internal compiler errors like rust-lang#68730 when non-ascii characters are given as input.

rust-lang#68735 partially addressed this problem, but only for one case. Moreover, its usage of `next_point()` does not actually align with what `bump_with()` expects. For example, given the token `>>=`, we should pass the span consisting of the final two characters `>=`, but `next_point()` advances the span beyond the end of the `=`.

This pull request instead computes the start of the new span by doing `start_point(self.token.span).hi()`. This matches `self.token.span.lo() + BytePos(1)` in the common case where the characters are ascii, and it gracefully handles multibyte characters.

Fixes rust-lang#68783.
bors added a commit that referenced this pull request Feb 6, 2020
Rollup of 9 pull requests

Successful merges:

 - #68691 (Remove `RefCell` usage from `ObligationForest`.)
 - #68751 (Implement `unused_parens` for `const` and `static` items)
 - #68788 (Towards unified `fn` grammar)
 - #68837 (Make associated item collection a query)
 - #68842 (or_patterns: add regression test for #68785)
 - #68844 (use def_path_str for missing_debug_impls message)
 - #68845 (stop using BytePos for computing spans in librustc_parse/parser/mod.rs)
 - #68869 (clean up E0271 explanation)
 - #68880 (Forbid using `0` as issue number)

Failed merges:

r? @ghost
@bors bors merged commit 9ac68e1 into rust-lang:master Feb 6, 2020
4 checks passed
4 checks passed
pr Build #20200205.13 succeeded
Details
pr (Linux mingw-check) Linux mingw-check succeeded
Details
pr (Linux x86_64-gnu-llvm-7) Linux x86_64-gnu-llvm-7 succeeded
Details
pr (Linux x86_64-gnu-tools) Linux x86_64-gnu-tools succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

4 participants
You can’t perform that action at this time.