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

Emit data::Impl in save-analysis #47657

Merged
merged 1 commit into from
Feb 11, 2018
Merged

Conversation

algesten
Copy link
Contributor

@algesten algesten commented Jan 22, 2018

As discussed on internals.rust-lang, this PR emits rls-data::Impl in the save-analysis.

A number of questions are outstanding:

  • A few ??? around row 356. We need to discuss what goes here, if anything.
  • Deriving id for impl using hashing. Is this going to clash with rustc defids?
  • Deriving id for impl using hashing. Is the conversion from 64 bit -> 32 bit problematic?
  • Need a new rls-data with an id field in Impl struct.
  • Need a new rls-data which derive Hash for ImplKind enum.

@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (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.

@algesten
Copy link
Contributor Author

It's specifically @nrc that needs to look at this PR to guide me in completing the work.

@jonhoo
Copy link
Contributor

jonhoo commented Jan 22, 2018

@algesten you can make that happen by writing r? @nrc

@algesten
Copy link
Contributor Author

r? @nrc

@rust-highfive rust-highfive assigned nrc and unassigned nikomatsakis Jan 22, 2018
@algesten
Copy link
Contributor Author

Thanks @jonhoo I was aware of the feature but unsure whether it applied to wip stuff.

@carols10cents carols10cents added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2018
@shepmaster
Copy link
Member

Ping from triage, @nrc!

@@ -1088,6 +1139,7 @@ pub fn process_crate<'l, 'tcx, H: SaveHandler>(
analysis,
span_utils: SpanUtils::new(&tcx.sess),
config: find_config(config),
impl_counter: Cell::new(0_u32),
Copy link
Member

Choose a reason for hiding this comment

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

You shouldn't need the _u32 here

self.dumper.dump_relation(impl_data);
if let super::Data::RelationData(rel, imp) = impl_data {
self.dumper.dump_relation(rel);
self.dumper.dump_impl(imp);
Copy link
Member

Choose a reason for hiding this comment

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

Using the cast macro rather than if let here is preferred.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrc but the cast macro is written for super::Data enclosing one value. Since this is the only data emitting two values, I though it cleaner make this an exeption rather than changing the macro to handle a tuple as well.

Copy link
Member

Choose a reason for hiding this comment

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

ok

kind: kind,
span: span,
value: String::new(), // ???
parent: None, // ??? enclosing module?
Copy link
Member

Choose a reason for hiding this comment

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

Yes, this should be the enclosing module

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok i fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrc sorry I feel stupid here. can I actually get the parent module? All other match arms of ast::ImplKind set parent: None. Is this case different?

id: impl_id,
kind: kind,
span: span,
value: String::new(), // ???
Copy link
Member

Choose a reason for hiding this comment

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

This is fine (I don't think we'll ever use it)

.collect(),
docs: "".to_string(), // ??? trait docs?
sig: None, // ??? trait sig?
attributes: vec![], // ??? trait attrs?
Copy link
Member

Choose a reason for hiding this comment

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

There might be attributes on the impl that should be recorded here (I think). We don't need anything else. For docs and sig, we can leave them as they are (though I would prefer to use only one way to make an empty string).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I fix

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nrc Unless I'm looking in the wrong place, there doesn't seem to be any attrs on the Impl level (there are on the ImplItem level).

https://manishearth.github.io/rust-internals-docs/syntax/ast/enum.ItemKind.html

let hash = hasher.finish();

// is this a good idea?
(hash ^ hash << 32) as u32
Copy link
Member

Choose a reason for hiding this comment

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

You can just use the 64 bit hash - iirc, we use 32 bits for a local id (NodeId) and 64 for a global id (DefId)

@nrc
Copy link
Member

nrc commented Jan 29, 2018

So, having thought this through a little bit, I think the hashing scheme might be overkill (sorry). We could just use the count of impls as the id, as long as clients ensure that impl ids are in a separate namespace from other ids, then I think everything will be OK (and that should be OK, since impl ids and def ids are not interchangeable).

@algesten
Copy link
Contributor Author

Ok. I remove the hashing.

@nrc
Copy link
Member

nrc commented Feb 1, 2018

OK, this all looks. I've merged your changes to rls-data and released that as version 0.15. Could you also squash your commits please?

@algesten
Copy link
Contributor Author

algesten commented Feb 1, 2018

Here's the new output. It looks ok.

{
  "impls": [
    {
      "id": 0,
      "kind": "Direct",
      "span": {
        "file_name": [
          115,
          114,
          99,
          47,
          109,
          97,
          105,
          110,
          46,
          114,
          115
        ],
        "byte_start": 234,
        "byte_end": 242,
        "line_start": 15,
        "line_end": 15,
        "column_start": 22,
        "column_end": 30
      },
      "value": "",
      "parent": null,
      "children": [
        {
          "krate": 0,
          "index": 22
        },
        {
          "krate": 0,
          "index": 24
        }
      ],
      "docs": "",
      "sig": null,
      "attributes": []
    }
  ]
}

@algesten
Copy link
Contributor Author

algesten commented Feb 1, 2018

@nrc I've updated to rls-data 0.15 and squashed the commits.

@algesten
Copy link
Contributor Author

algesten commented Feb 1, 2018

Hm. maybe i rebase..

@nrc
Copy link
Member

nrc commented Feb 2, 2018

I think you don't want to change the submodules which are included in this PR? But you probably do need to build (and maybe update) in order to get rls-data 0.15 into the Cargo.lock

@algesten
Copy link
Contributor Author

algesten commented Feb 2, 2018

Oh. I check.

@algesten
Copy link
Contributor Author

algesten commented Feb 2, 2018

@nrc sorted. submodule free and one Cargo.lock.

@algesten
Copy link
Contributor Author

algesten commented Feb 2, 2018

and rebased off master again.

@nrc
Copy link
Member

nrc commented Feb 5, 2018

Thanks!

@bors: r+

@bors
Copy link
Contributor

bors commented Feb 5, 2018

📌 Commit 38c517c has been approved by nrc

@bors
Copy link
Contributor

bors commented Feb 5, 2018

🔒 Merge conflict

@algesten
Copy link
Contributor Author

algesten commented Feb 5, 2018

@nrc do i rebase off master again?

@kennytm
Copy link
Member

kennytm commented Feb 5, 2018

@algesten Yes please rebase. Also, remove the [wip] from the title if it is ready for merging.

@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Feb 7, 2018
@kennytm
Copy link
Member

kennytm commented Feb 7, 2018

Why this PR is always merge-conflicting 😂

(Please rebase again)

@algesten
Copy link
Contributor Author

Done

@shepmaster
Copy link
Member

@bors r=nrc

@bors
Copy link
Contributor

bors commented Feb 10, 2018

📌 Commit 9a6afa8 has been approved by nrc

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 10, 2018
@bors
Copy link
Contributor

bors commented Feb 10, 2018

⌛ Testing commit 9a6afa8 with merge eb0fdb6...

bors added a commit that referenced this pull request Feb 10, 2018
Emit data::Impl in save-analysis

As discussed on [internals.rust-lang](https://internals.rust-lang.org/t/rustdoc2-rls-analysis-and-the-compiler-help-wanted/6592/5), this PR emits `rls-data::Impl` in the save-analysis.

A number of questions are outstanding:

- [x] A few `???` around row 356. We need to discuss what goes here, if anything.
- [ ] ~~Deriving `id` for impl using hashing. Is this going to clash with rustc defids?~~
- [ ] ~~Deriving `id` for impl using hashing. Is the conversion from 64 bit -> 32 bit problematic?~~
- [x] Need a new rls-data with an `id` field in `Impl` struct.
- [ ] ~~Need a new rls-data which `derive` `Hash` for `ImplKind` enum.~~
@bors
Copy link
Contributor

bors commented Feb 10, 2018

💔 Test failed - status-appveyor

@bors 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 Feb 10, 2018
@kennytm
Copy link
Member

kennytm commented Feb 10, 2018

@bors retry #48116

@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 Feb 10, 2018
@bors
Copy link
Contributor

bors commented Feb 11, 2018

⌛ Testing commit 9a6afa8 with merge 0bb8935...

bors added a commit that referenced this pull request Feb 11, 2018
Emit data::Impl in save-analysis

As discussed on [internals.rust-lang](https://internals.rust-lang.org/t/rustdoc2-rls-analysis-and-the-compiler-help-wanted/6592/5), this PR emits `rls-data::Impl` in the save-analysis.

A number of questions are outstanding:

- [x] A few `???` around row 356. We need to discuss what goes here, if anything.
- [ ] ~~Deriving `id` for impl using hashing. Is this going to clash with rustc defids?~~
- [ ] ~~Deriving `id` for impl using hashing. Is the conversion from 64 bit -> 32 bit problematic?~~
- [x] Need a new rls-data with an `id` field in `Impl` struct.
- [ ] ~~Need a new rls-data which `derive` `Hash` for `ImplKind` enum.~~
@bors
Copy link
Contributor

bors commented Feb 11, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: nrc
Pushing 0bb8935 to master...

@bors bors merged commit 9a6afa8 into rust-lang:master Feb 11, 2018
kennytm-githubbot added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Feb 11, 2018
Tested on commit rust-lang/rust@0bb8935.

💔 rls on windows: test-pass → test-fail (cc @nrc).
💔 rls on linux: test-pass → test-fail (cc @nrc).
@kennytm
Copy link
Member

kennytm commented Feb 11, 2018

@nrc @algesten FYI the rls tests failed on this PR.

[01:26:35] failures:
[01:26:35] 
[01:26:35] ---- test::test_find_impls stdout ----
[01:26:35] 	expect_messages:
[01:26:35]   results: [
[01:26:35]     "{\"jsonrpc\":\"2.0\",\"id\":0,\"result\":{\"capabilities\":{\"textDocumentSync\":2,\"hoverProvider\":true,\"completionProvider\":{\"resolveProvider\":true,\"triggerCharacters\":[\".\",\":\"]},\"definitionProvider\":true,\"referencesProvider\":true,\"documentHighlightProvider\":true,\"documentSymbolProvider\":true,\"workspaceSymbolProvider\":true,\"codeActionProvider\":true,\"documentFormattingProvider\":true,\"documentRangeFormattingProvider\":false,\"renameProvider\":true,\"executeCommandProvider\":{\"commands\":[\"rls.applySuggestion\",\"rls.deglobImports\"]}}}}",
[01:26:35]     "{\"jsonrpc\":\"2.0\",\"method\":\"rustDocument/beginBuild\"}",
[01:26:35]     "{\"jsonrpc\":\"2.0\",\"method\":\"rustDocument/diagnosticsBegin\"}",
[01:26:35]     "{\"jsonrpc\":\"2.0\",\"method\":\"rustDocument/diagnosticsEnd\"}"
[01:26:35] ],
[01:26:35]   expected: [
[01:26:35]     ExpectedMessage {
[01:26:35]         id: Some(
[01:26:35]             0
[01:26:35]         ),
[01:26:35]         contains: [
[01:26:35]             "capabilities"
[01:26:35]         ]
[01:26:35]     },
[01:26:35]     ExpectedMessage {
[01:26:35]         id: None,
[01:26:35]         contains: [
[01:26:35]             "beginBuild"
[01:26:35]         ]
[01:26:35]     },
[01:26:35]     ExpectedMessage {
[01:26:35]         id: None,
[01:26:35]         contains: [
[01:26:35]             "diagnosticsBegin"
[01:26:35]         ]
[01:26:35]     },
[01:26:35]     ExpectedMessage {
[01:26:35]         id: None,
[01:26:35]         contains: [
[01:26:35]             "diagnosticsEnd"
[01:26:35]         ]
[01:26:35]     }
[01:26:35] ]
[01:26:35] expect_messages:
[01:26:35]   results: [
[01:26:35]     "{\"jsonrpc\":\"2.0\",\"id\":1,\"result\":[{\"uri\":\"file:///checkout/src/tools/rls/test_data/find_impls/src/main.rs\",\"range\":{\"start\":{\"line\":18,\"character\":15},\"end\":{\"line\":18,\"character\":18}}}]}"
[01:26:35] ],
[01:26:35]   expected: [
[01:26:35]     ExpectedMessage {
[01:26:35]         id: Some(
[01:26:35]             1
[01:26:35]         ),
[01:26:35]         contains: [
[01:26:35]             "\"range\":{\"start\":{\"line\":18,\"character\":15},\"end\":{\"line\":18,\"character\":18}}",
[01:26:35]             "\"range\":{\"start\":{\"line\":19,\"character\":12},\"end\":{\"line\":19,\"character\":15}}"
[01:26:35]         ]
[01:26:35]     }
[01:26:35] ]
[01:26:35] thread 'test::test_find_impls' panicked at 'Could not find `"range":{"start":{"line":19,"character":12},"end":{"line":19,"character":15}}` in `{"jsonrpc":"2.0","id":1,"result":[{"uri":"file:///checkout/src/tools/rls/test_data/find_impls/src/main.rs","range":{"start":{"line":18,"character":15},"end":{"line":18,"character":18}}}]}`', libcore/option.rs:917:5
[01:26:35] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[01:26:35] 
[01:26:35] 
[01:26:35] failures:
[01:26:35]     test::test_find_impls
[01:26:35] 
[01:26:35] test result: FAILED. 39 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
[01:26:35] 
[01:26:35] error: test failed, to rerun pass '--bin rls'

@nrc
Copy link
Member

nrc commented Feb 11, 2018

@kennytm this is expected (I think) and should be solved by updating the RLS and then updating the version in the Repo

@algesten algesten deleted the save-analysis-impls branch March 3, 2018 09:48
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants