Move lev_distance to rustc_ast, make non-generic #79000

sivadeilra · 2020-11-12T20:05:47Z

rustc_ast currently has a few dependencies on rustc_lexer. Ideally, an AST
would not have any dependency its lexer, for minimizing
design-time dependencies. Breaking this dependency would also have practical
benefits, since modifying rustc_lexer would not trigger a rebuild of rustc_ast.

This commit does not remove the rustc_ast --> rustc_lexer dependency,
but it does remove one of the sources of this dependency, which is the
code that handles fuzzy matching between symbol names for making suggestions
in diagnostics. Since that code depends only on Symbol, it is easy to move
it to rustc_span. It might even be best to move it to a separate crate,
since other tools such as Cargo use the same algorithm, and have simply
contain a duplicate of the code.

This changes the signature of find_best_match_for_name so that it is no
longer generic over its input. I checked the optimized binaries, and this
function was duplicated for nearly every call site, because most call sites
used short-lived iterator chains, generic over Map and such. But there's
no good reason for a function like this to be generic, since all it does
is immediately convert the generic input (the Iterator impl) to a concrete
Vec. This has all of the costs of generics (duplicated method bodies)
with no benefit.

Changing find_best_match_for_name to be non-generic removed about 10KB of
code from the optimized binary. I know it's a drop in the bucket, but we have
to start reducing binary size, and beginning to tame over-use of generics
is part of that.

rust-highfive · 2020-11-12T20:05:50Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

rust-log-analyzer · 2020-11-12T20:11:56Z

The job mingw-check 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.

[command]/usr/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :
[command]/usr/bin/git config --local http.https://github.com/.extraheader AUTHORIZATION: basic ***
##[endgroup]
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=2 origin +dc793e66ca1831a040a7b514b362f584ff02854d:refs/remotes/pull/79000/merge
---
    Checking clippy_lints v0.0.212 (/checkout/src/tools/clippy/clippy_lints)
error[E0308]: mismatched types
   --> src/tools/clippy/clippy_lints/src/attrs.rs:430:29
    |
430 | ...                   symbols.iter(),
    |                       ^^^^^^^^^^^^^^ expected `&[Symbol]`, found struct `std::slice::Iter`
    |
    = note: expected reference `&[Symbol]`
                  found struct `std::slice::Iter<'_, Symbol>`
error: aborting due to previous error

For more information about this error, try `rustc --explain E0308`.
error: could not compile `clippy_lints`
---
== clock drift check ==
  local time: Thu Nov 12 20:11:45 UTC 2020
  network time: Thu, 12 Nov 2020 09:14:37 GMT
== end clock drift check ==
##[error]Process completed with exit code 1.
[command]/usr/bin/git version
git version 2.29.2
[command]/usr/bin/git config --local --name-only --get-regexp core\.sshCommand
[command]/usr/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'core\.sshCommand' && git config --local --unset-all 'core.sshCommand' || :

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 @rust-lang/infra. (Feature Requests)

rust-log-analyzer · 2020-11-12T23:28:30Z

The job mingw-check 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.

[command]/usr/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :
[command]/usr/bin/git config --local http.https://github.com/.extraheader AUTHORIZATION: basic ***
##[endgroup]
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=2 origin +8bcd6d407b79021af614497296a568c5bf721032:refs/remotes/pull/79000/merge
---
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_span/src/lev_distance/tests.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
Diff in /checkout/compiler/rustc_span/src/lev_distance/tests.rs at line 29:
             Some(Symbol::intern("aaab"))
         );
-        assert_eq!(
-        assert_eq!(
-            find_best_match_for_name(&input, Symbol::intern("1111111111"), None),
-            None
-        );
+        assert_eq!(find_best_match_for_name(&input, Symbol::intern("1111111111"), None), None);
 
         let input = vec![Symbol::intern("aAAA")];
         assert_eq!(
Build completed unsuccessfully in 0:00:13
== clock drift check ==
  local time: Thu Nov 12 23:28:00 UTC 2020
  network time: Thu, 12 Nov 2020 09:26:07 GMT

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 @rust-lang/infra. (Feature Requests)

rust-log-analyzer · 2020-11-13T00:12:14Z

The job mingw-check 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.

[command]/usr/bin/git submodule foreach --recursive git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :
[command]/usr/bin/git config --local http.https://github.com/.extraheader AUTHORIZATION: basic ***
##[endgroup]
##[group]Fetching the repository
[command]/usr/bin/git -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=2 origin +4664b982e6f1908bf49adbca1b0c87b64b9383cf:refs/remotes/pull/79000/merge
---
configure: rust.channel         := nightly
configure: rust.debug-assertions := True
configure: llvm.assertions      := True
configure: dist.missing-tools   := True
configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
configure: writing `config.toml` in current directory
configure: 
configure: run `python /checkout/x.py --help`
configure: 
---
skip untracked path cpu-usage.csv during rustfmt invocations
skip untracked path src/doc/book/ during rustfmt invocations
skip untracked path src/doc/rust-by-example/ during rustfmt invocations
skip untracked path src/llvm-project/ during rustfmt invocations
Diff in /checkout/compiler/rustc_span/src/lev_distance.rs at line 56:
         .iter()
         .filter_map(|&name| {
             let dist = lev_distance(lookup, &name.as_str());
-            if dist <= max_dist {
-                Some((name, dist))
-                None
-            }
-            }
+            if dist <= max_dist { Some((name, dist)) } else { None }
         })
Running `"/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/rustfmt" "--config-path" "/checkout" "--edition" "2018" "--unstable-features" "--skip-children" "--check" "/checkout/compiler/rustc_span/src/lev_distance.rs"` failed.
If you're running `tidy`, try again with `--bless`. Or, if you just want to format code, run `./x.py fmt` instead.
         // Here we are collecting the next structure:
         // (case_insensitive_match, (levenshtein_match, levenshtein_distance))
Build completed unsuccessfully in 0:00:15
== clock drift check ==
  local time: Fri Nov 13 00:12:04 UTC 2020
  network time: Thu, 12 Nov 2020 00:26:09 GMT

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 @rust-lang/infra. (Feature Requests)

sivadeilra · 2020-11-16T20:51:06Z

Ping?

matthewjasper · 2020-11-22T22:30:43Z

@bors r+

bors · 2020-11-22T22:30:44Z

📌 Commit 954f7e68a3ccefbdc7e3f7b2951c42dcd11963fc has been approved by matthewjasper

bors · 2020-11-23T09:23:01Z

⌛ Testing commit 954f7e68a3ccefbdc7e3f7b2951c42dcd11963fc with merge ba97da43eb217b8a82764e1773459c116155cc63...

bors · 2020-11-23T09:45:15Z

💔 Test failed - checks-actions

matthewjasper · 2020-11-24T20:29:24Z

@bors r+

bors · 2020-11-24T20:29:26Z

📌 Commit 63117ba70b2276f2ef796870af1d41ca72a602fc has been approved by matthewjasper

sivadeilra · 2020-11-24T21:16:14Z

The update 63117ba was just a rebase. There were no merge conflicts.

bors · 2020-11-24T23:24:29Z

⌛ Testing commit 63117ba70b2276f2ef796870af1d41ca72a602fc with merge e576ed0a8f3fbc79cb86ccf7075a18f0000d34f2...

Mark-Simulacrum · 2020-11-24T23:28:57Z

@bors r- retry

Can we squash commit history here into one commit? You can see https://rustc-dev-guide.rust-lang.org/git.html#advanced-rebasing for some help on how to do that or we can do it for you if that'd be helpful :)

sivadeilra · 2020-11-25T00:10:25Z

Certainly. I thought GitHub handled that as one of its forms of committing? I know ADO does, but I just assumed GH did, too.

rustc_ast currently has a few dependencies on rustc_lexer. Ideally, an AST would not have any dependency its lexer, for minimizing unnecessarily design-time dependencies. Breaking this dependency would also have practical benefits, since modifying rustc_lexer would not trigger a rebuild of rustc_ast. This commit does not remove the rustc_ast --> rustc_lexer dependency, but it does remove one of the sources of this dependency, which is the code that handles fuzzy matching between symbol names for making suggestions in diagnostics. Since that code depends only on Symbol, it is easy to move it to rustc_span. It might even be best to move it to a separate crate, since other tools such as Cargo use the same algorithm, and have simply contain a duplicate of the code. This changes the signature of find_best_match_for_name so that it is no longer generic over its input. I checked the optimized binaries, and this function was duplicated at nearly every call site, because most call sites used short-lived iterator chains, generic over Map and such. But there's no good reason for a function like this to be generic, since all it does is immediately convert the generic input (the Iterator impl) to a concrete Vec<Symbol>. This has all of the costs of generics (duplicated method bodies) with no benefit. Changing find_best_match_for_name to be non-generic removed about 10KB of code from the optimized binary. I know it's a drop in the bucket, but we have to start reducing binary size, and beginning to tame over-use of generics is part of that.

sivadeilra · 2020-11-25T00:13:31Z

Squashed, fetched, rebased, push-f'd. :)

wesleywiser · 2020-11-26T01:42:54Z

@bors r+

bors · 2020-11-26T01:42:55Z

📌 Commit 5481c1b has been approved by wesleywiser

bors · 2020-11-26T01:42:56Z

🌲 The tree is currently closed for pull requests below priority 500, this pull request will be tested once the tree is reopened

…as-schievink Rollup of 10 pull requests Successful merges: - rust-lang#77758 (suggest turbofish syntax for uninferred const arguments) - rust-lang#79000 (Move lev_distance to rustc_ast, make non-generic) - rust-lang#79362 (Lower patterns before using the bound variable) - rust-lang#79365 (Upgrades the coverage map to Version 4) - rust-lang#79402 (Fix typos) - rust-lang#79412 (Clean up rustdoc tests by removing unnecessary features) - rust-lang#79413 (Fix persisted doctests on Windows / when using workspaces) - rust-lang#79420 (Fixes a word typo in librustdoc) - rust-lang#79421 (Fix docs formatting for `thir::pattern::_match`) - rust-lang#79428 (Fixup compiler docs) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rust-highfive assigned matthewjasper Nov 12, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 12, 2020

jyn514 added A-parser Area: The parsing of Rust source code to an AST T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. C-cleanup Category: PRs that clean code up or issues documenting cleanup. labels Nov 12, 2020

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 Nov 22, 2020

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 23, 2020

sivadeilra force-pushed the user/ardavis/lev_distance branch from 954f7e6 to 63117ba Compare November 24, 2020 05:41

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 Nov 24, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 24, 2020

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 24, 2020

sivadeilra force-pushed the user/ardavis/lev_distance branch from 63117ba to 5481c1b Compare November 25, 2020 00:13

This was referenced Nov 26, 2020

Rollup of 11 pull requests #79440

Closed

Rollup of 10 pull requests #79441

Merged

bors merged commit 6fcd589 into rust-lang:master Nov 26, 2020

rustbot added this to the 1.50.0 milestone Nov 26, 2020

sivadeilra deleted the user/ardavis/lev_distance branch November 27, 2020 02:53

jyn514 mentioned this pull request Nov 27, 2020

When pushing to an r+'d PR, bors does not update the labels rust-lang/homu#9

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Move lev_distance to rustc_ast, make non-generic #79000

Move lev_distance to rustc_ast, make non-generic #79000

sivadeilra commented Nov 12, 2020 •

edited

Loading

rust-highfive commented Nov 12, 2020

rust-log-analyzer commented Nov 12, 2020

rust-log-analyzer commented Nov 12, 2020

rust-log-analyzer commented Nov 13, 2020

sivadeilra commented Nov 16, 2020

matthewjasper commented Nov 22, 2020

bors commented Nov 22, 2020

bors commented Nov 23, 2020

bors commented Nov 23, 2020

matthewjasper commented Nov 24, 2020

bors commented Nov 24, 2020

sivadeilra commented Nov 24, 2020

bors commented Nov 24, 2020

Mark-Simulacrum commented Nov 24, 2020

sivadeilra commented Nov 25, 2020

sivadeilra commented Nov 25, 2020

wesleywiser commented Nov 26, 2020

bors commented Nov 26, 2020

bors commented Nov 26, 2020

Move lev_distance to rustc_ast, make non-generic #79000

Move lev_distance to rustc_ast, make non-generic #79000

Conversation

sivadeilra commented Nov 12, 2020 • edited Loading

rust-highfive commented Nov 12, 2020

rust-log-analyzer commented Nov 12, 2020

rust-log-analyzer commented Nov 12, 2020

rust-log-analyzer commented Nov 13, 2020

sivadeilra commented Nov 16, 2020

matthewjasper commented Nov 22, 2020

bors commented Nov 22, 2020

bors commented Nov 23, 2020

bors commented Nov 23, 2020

matthewjasper commented Nov 24, 2020

bors commented Nov 24, 2020

sivadeilra commented Nov 24, 2020

bors commented Nov 24, 2020

Mark-Simulacrum commented Nov 24, 2020

sivadeilra commented Nov 25, 2020

sivadeilra commented Nov 25, 2020

wesleywiser commented Nov 26, 2020

bors commented Nov 26, 2020

bors commented Nov 26, 2020

sivadeilra commented Nov 12, 2020 •

edited

Loading