Export attributes in save-analysis data #39820

jonasbb · 2017-02-14T16:30:32Z

Since this is my first pull-request to rust, I would like to get some feedback about obvious errors in this implementation.

I would like to change the save-analysis data to include arbitrary attribute data.
A use-case I have in mind for this is identifying functions with #[test] annotations such that tools like rls can offer a test-runner feature. I described my idea here rls#173.

My changes contain:

track a vector of attributes in the various *Data types in data.rs and external_data.rs
implement lowering for Attribute and MetaItem
adjust JsonDumper to print the attributes

In the lowering of Attribute I remove the distinction between MetaItem and NestedMetaItem. I did this because this distinction is somewhat confusing. For example, NestedMetaItemKind::Literal has two identical spans, because both NestedMetaItem and Lit are defined as Spanned<_>.
My model is strictly more general, as it allows an LitKind instead of a Symbol for MetaItem and Symbols are converted into a cooked string. As a consumer of the save-analysis data this shouldn't affect you much.

Example json output of #[test] annotation:

"attributes": [
  {
    "value": {
      "name": {
        "variant": "Str",
        "fields": [
          "test",
          "Cooked"
        ]     
      },    
      "kind": "Literal",
      "span": {
        "file_name": "test.rs",
        "byte_start": 2,
        "byte_end": 6,
        "line_start": 1,
        "line_end": 1,
        "column_start": 3,
        "column_end": 7
      }     
    },    
    "span": {
      "file_name": "test.rs",
      "byte_start": 0,
      "byte_end": 7,
      "line_start": 1,
      "line_end": 1,
      "column_start": 1,
      "column_end": 8
    }
  }
]

rust-highfive · 2017-02-14T16:30:49Z

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

nrc

This looks really good! I wonder if the data we output should be simpler though - it seems a bit too close to the AST atm.

nrc · 2017-02-22T01:55:41Z

src/librustc_save_analysis/external_data.rs

+/// A single item as part of an attribute
+#[derive(Clone, Debug, RustcEncodable)]
+pub struct AttributeItem {
+    name: LitKind,


This should probably just be a string, rather than coding it as a LitKind

nrc · 2017-02-22T01:57:12Z

src/librustc_save_analysis/external_data.rs

+    /// List meta item.
+    ///
+    /// E.g. the `derive(..)` as in `#[derive(..)]`
+    List(Vec<AttributeItem>),


I feel like this is too much structure for save-analysis - will we need it all in the RLS? Could we do something simpler?

jonasbb · 2017-02-22T15:16:56Z

@nrc Thank you for the feedback. I agree that it is very close to the AST. While implementing the feature I found out that it is possible to have arbitrary literals in the attributes (using the attr_literals feature). I wanted to be as general as possible, therefore I chose the design so close to the AST.

This allows to answer any query about attributes. A query which would need this information would be "All types which derive Serializable". I am not sure if this is needed in RLS though.

I see two ways to simplify it:

Remove support for attr_literals in save-analysis. They could be converted into strings so that any save-analysis consumer must parse them again. I don't think this is a good solution if the goal of rust is to support literals in attributes.
Only store the name of the attribute in save-analysis data. The name must always be an identifier and cannot ever be a literal. This would also remove the need of AttributeItemKind.

I am fine with implementing option 2. because this would be sufficient to satisfy my requirement. Are there other consumers besides RLS which would like this information?

nrc · 2017-02-23T01:25:48Z

The long term plan for attributes is that they are basically just a name and a bunch of tokens. So I'm not sure that it makes sense to try and preserve any structure beyond a string.

I think any semantic queries about attributes would be better served by analysing the result of the attribute rather than the attribute itself. E.g., rather than looking for derives, look for the impls generated by the derives.

jonasbb · 2017-02-23T22:59:49Z

@nrc Reducing attributes to be strings is quite nice I think. I use pprust::attribute_to_string to create a string and then strip the surrounding #[/#![ and ] off it.

nrc · 2017-02-24T02:13:06Z

src/librustc_save_analysis/external_data.rs

+
+    fn lower(mut self, tcx: TyCtxt) -> Attribute {
+        // strip #[] and #![] from the original attributes
+        self.style = ast::AttrStyle::Outer;


Heh, this is a neat little hack :-)

I think the comment could explain more clearer what is happening here though

nrc · 2017-02-24T02:13:27Z

src/librustc_save_analysis/external_data.rs

+        // strip #[] and #![] from the original attributes
+        self.style = ast::AttrStyle::Outer;
+        let value = pprust::attribute_to_string(&self);
+        // #[] are all ASCII which makes this slice save


The grammar in this comment seems not right to me.

nrc · 2017-02-24T02:14:34Z

src/librustc_save_analysis/lib.rs

@@ -228,6 +232,7 @@ impl<'l, 'tcx: 'l> SaveContext<'l, 'tcx> {
                    visibility: From::from(&item.vis),
                    docs: docs_for_attrs(&item.attrs),
                    sig: self.sig_base(item),
+                    attributes: remove_docs_from_attrs(&item.attrs),


could the remove_docs_from_attrs call be moved to the lower function so it is not duplicated?

Certainly. I followed the usage of docs_for_attrs here, which does something very similar, except for comments instead of attributes. They are even more similar now, because attributes are lowered to (String, Span) pairs. Calling this function (or even the lowering) earlier reduces the amount of data which needs to be copies around.

nrc · 2017-03-07T04:49:32Z

Thanks for the changes!

@bors: r+

bors · 2017-03-07T04:49:33Z

📌 Commit 5bfa0f3 has been approved by nrc

bors · 2017-03-08T12:17:16Z

⌛ Testing commit 5bfa0f3 with merge 05076d9...

bors · 2017-03-08T13:00:08Z

💔 Test failed - status-travis

arielb1 · 2017-03-08T14:18:55Z

error: failed to run custom build command for `compiler_builtins v0.0.0 (file:///checkout/src/libcompiler_builtins)`
...
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: Error { repr: Os { code: 17, message: "File exists" } }', /buildslave/rust-buildbot/slave/beta-dist-rustc-linux/build/src/libcore/result.rs:868

arielb1 · 2017-03-08T14:19:11Z

cc @alexcrichton

alexcrichton · 2017-03-08T15:07:44Z

@bors: retry

Spurious "file exists" exception in libcompiler_builtins build script #40358

bors · 2017-03-09T10:41:19Z

🔒 Merge conflict

arielb1 · 2017-03-09T11:26:36Z

@bors retry

bors · 2017-03-09T11:26:40Z

🔒 Merge conflict

alexcrichton · 2017-03-09T15:02:46Z

@bors: retry

Export attributes in save-analysis data Since this is my first pull-request to rust, I would like to get some feedback about obvious errors in this implementation. I would like to change the save-analysis data to include arbitrary attribute data. A use-case I have in mind for this is identifying functions with `#[test]` annotations such that tools like rls can offer a test-runner feature. I described my idea here [rls#173](rust-lang/rls#173). My changes contain: 1. track a vector of attributes in the various `*Data` types in `data.rs` and `external_data.rs` 2. implement lowering for `Attribute` and `MetaItem` 3. adjust `JsonDumper` to print the attributes In the lowering of `Attribute` I remove the distinction between `MetaItem` and `NestedMetaItem`. I did this because this distinction is somewhat confusing. For example, `NestedMetaItemKind::Literal` has two identical spans, because both `NestedMetaItem` and `Lit` are defined as `Spanned<_>`. My model is strictly more general, as it allows an `LitKind` instead of a `Symbol` for `MetaItem` and `Symbol`s are converted into a cooked string. As a consumer of the save-analysis data this shouldn't affect you much. Example json output of `#[test]` annotation: ``` "attributes": [ { "value": { "name": { "variant": "Str", "fields": [ "test", "Cooked" ] }, "kind": "Literal", "span": { "file_name": "test.rs", "byte_start": 2, "byte_end": 6, "line_start": 1, "line_end": 1, "column_start": 3, "column_end": 7 } }, "span": { "file_name": "test.rs", "byte_start": 0, "byte_end": 7, "line_start": 1, "line_end": 1, "column_start": 1, "column_end": 8 } } ] ```

bors · 2017-03-09T19:52:44Z

🔒 Merge conflict

alexcrichton · 2017-03-09T19:58:25Z

@bors: retry

bors · 2017-03-10T07:49:01Z

🔒 Merge conflict

alexcrichton · 2017-03-10T15:04:48Z

@bors: retry

Export attributes in save-analysis data Since this is my first pull-request to rust, I would like to get some feedback about obvious errors in this implementation. I would like to change the save-analysis data to include arbitrary attribute data. A use-case I have in mind for this is identifying functions with `#[test]` annotations such that tools like rls can offer a test-runner feature. I described my idea here [rls#173](rust-lang/rls#173). My changes contain: 1. track a vector of attributes in the various `*Data` types in `data.rs` and `external_data.rs` 2. implement lowering for `Attribute` and `MetaItem` 3. adjust `JsonDumper` to print the attributes In the lowering of `Attribute` I remove the distinction between `MetaItem` and `NestedMetaItem`. I did this because this distinction is somewhat confusing. For example, `NestedMetaItemKind::Literal` has two identical spans, because both `NestedMetaItem` and `Lit` are defined as `Spanned<_>`. My model is strictly more general, as it allows an `LitKind` instead of a `Symbol` for `MetaItem` and `Symbol`s are converted into a cooked string. As a consumer of the save-analysis data this shouldn't affect you much. Example json output of `#[test]` annotation: ``` "attributes": [ { "value": { "name": { "variant": "Str", "fields": [ "test", "Cooked" ] }, "kind": "Literal", "span": { "file_name": "test.rs", "byte_start": 2, "byte_end": 6, "line_start": 1, "line_end": 1, "column_start": 3, "column_end": 7 } }, "span": { "file_name": "test.rs", "byte_start": 0, "byte_end": 7, "line_start": 1, "line_end": 1, "column_start": 1, "column_end": 8 } } ] ```

Some annotations like the "test" annotations might be of interest for other projects, especially rls. Export all attributes in a new attributes item.

Remove the AST structure

alexcrichton · 2017-03-10T16:02:58Z

@bors: r=nrc

bors · 2017-03-10T16:02:59Z

📌 Commit db35604 has been approved by nrc

Export attributes in save-analysis data Since this is my first pull-request to rust, I would like to get some feedback about obvious errors in this implementation. I would like to change the save-analysis data to include arbitrary attribute data. A use-case I have in mind for this is identifying functions with `#[test]` annotations such that tools like rls can offer a test-runner feature. I described my idea here [rls#173](rust-lang/rls#173). My changes contain: 1. track a vector of attributes in the various `*Data` types in `data.rs` and `external_data.rs` 2. implement lowering for `Attribute` and `MetaItem` 3. adjust `JsonDumper` to print the attributes In the lowering of `Attribute` I remove the distinction between `MetaItem` and `NestedMetaItem`. I did this because this distinction is somewhat confusing. For example, `NestedMetaItemKind::Literal` has two identical spans, because both `NestedMetaItem` and `Lit` are defined as `Spanned<_>`. My model is strictly more general, as it allows an `LitKind` instead of a `Symbol` for `MetaItem` and `Symbol`s are converted into a cooked string. As a consumer of the save-analysis data this shouldn't affect you much. Example json output of `#[test]` annotation: ``` "attributes": [ { "value": { "name": { "variant": "Str", "fields": [ "test", "Cooked" ] }, "kind": "Literal", "span": { "file_name": "test.rs", "byte_start": 2, "byte_end": 6, "line_start": 1, "line_end": 1, "column_start": 3, "column_end": 7 } }, "span": { "file_name": "test.rs", "byte_start": 0, "byte_end": 7, "line_start": 1, "line_end": 1, "column_start": 1, "column_end": 8 } } ] ```

Rollup of 38 pull requests - Successful merges: #39202, #39820, #39918, #39921, #40092, #40146, #40199, #40225, #40239, #40257, #40259, #40261, #40277, #40278, #40287, #40297, #40311, #40315, #40319, #40324, #40336, #40340, #40344, #40345, #40367, #40369, #40372, #40373, #40379, #40385, #40386, #40389, #40400, #40404, #40410, #40422, #40423, #40424 - Failed merges: #40220, #40329, #40426

rust-highfive assigned nrc Feb 14, 2017

nrc suggested changes Feb 22, 2017

View reviewed changes

nrc reviewed Feb 24, 2017

View reviewed changes

frewsxcv mentioned this pull request Mar 9, 2017

Rollup of 10 pull requests #40393

Closed

alexcrichton mentioned this pull request Mar 10, 2017

Rollup of 36 pull requests #40418

Closed

jonasbb added 2 commits March 10, 2017 08:02

Export attributes in save-analysis data

a07c9a2

Some annotations like the "test" annotations might be of interest for other projects, especially rls. Export all attributes in a new attributes item.

Store attributes as strings

203d227

Remove the AST structure

Move remove_docs_from_attrs into lowering step

db35604

alexcrichton mentioned this pull request Mar 10, 2017

Rollup of 35 pull requests #40420

Closed

arielb1 mentioned this pull request Mar 10, 2017

Rollup of 24 pull requests #40427

Closed

arielb1 mentioned this pull request Mar 10, 2017

Rollup of 26 pull requests #40428

Closed

arielb1 mentioned this pull request Mar 10, 2017

Rollup of 29 pull requests #40429

Closed

alexcrichton mentioned this pull request Mar 10, 2017

Rollup of 38 pull requests #40432

Merged

bors merged commit db35604 into rust-lang:master Mar 11, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Export attributes in save-analysis data #39820

Export attributes in save-analysis data #39820

jonasbb commented Feb 14, 2017

rust-highfive commented Feb 14, 2017

nrc left a comment

nrc Feb 22, 2017

nrc Feb 22, 2017

jonasbb commented Feb 22, 2017

nrc commented Feb 23, 2017

jonasbb commented Feb 23, 2017

nrc Feb 24, 2017

nrc Feb 24, 2017

nrc Feb 24, 2017

jonasbb Feb 24, 2017

nrc commented Mar 7, 2017

bors commented Mar 7, 2017

bors commented Mar 8, 2017

bors commented Mar 8, 2017

arielb1 commented Mar 8, 2017

arielb1 commented Mar 8, 2017

alexcrichton commented Mar 8, 2017

bors commented Mar 9, 2017

arielb1 commented Mar 9, 2017

bors commented Mar 9, 2017

alexcrichton commented Mar 9, 2017

bors commented Mar 9, 2017

alexcrichton commented Mar 9, 2017

bors commented Mar 10, 2017

alexcrichton commented Mar 10, 2017

alexcrichton commented Mar 10, 2017

bors commented Mar 10, 2017

Export attributes in save-analysis data #39820

Export attributes in save-analysis data #39820

Conversation

jonasbb commented Feb 14, 2017

rust-highfive commented Feb 14, 2017

nrc left a comment

Choose a reason for hiding this comment

nrc Feb 22, 2017

Choose a reason for hiding this comment

nrc Feb 22, 2017

Choose a reason for hiding this comment

jonasbb commented Feb 22, 2017

nrc commented Feb 23, 2017

jonasbb commented Feb 23, 2017

nrc Feb 24, 2017

Choose a reason for hiding this comment

nrc Feb 24, 2017

Choose a reason for hiding this comment

nrc Feb 24, 2017

Choose a reason for hiding this comment

jonasbb Feb 24, 2017

Choose a reason for hiding this comment

nrc commented Mar 7, 2017

bors commented Mar 7, 2017

bors commented Mar 8, 2017

bors commented Mar 8, 2017

arielb1 commented Mar 8, 2017

arielb1 commented Mar 8, 2017

alexcrichton commented Mar 8, 2017

bors commented Mar 9, 2017

arielb1 commented Mar 9, 2017

bors commented Mar 9, 2017

alexcrichton commented Mar 9, 2017

bors commented Mar 9, 2017

alexcrichton commented Mar 9, 2017

bors commented Mar 10, 2017

alexcrichton commented Mar 10, 2017

alexcrichton commented Mar 10, 2017

bors commented Mar 10, 2017