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

Tuple struct index inference #1117

Merged
merged 2 commits into from
Apr 5, 2019
Merged

Tuple struct index inference #1117

merged 2 commits into from
Apr 5, 2019

Conversation

robojumper
Copy link
Contributor

@robojumper robojumper commented Apr 5, 2019

The first commit adds a helper struct ast::FieldKind to facilitate inference.

The second commit adds a slightly modified test from #1109 while mentioning that there is a problem with how we're handling tuple indexing / floats.

cc #1109

@robojumper robojumper changed the title Tuple struct index inference [WIP] Tuple struct index inference Apr 5, 2019
Copy link
Member

@flodiebold flodiebold left a comment

Choose a reason for hiding this comment

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

LGTM, I don't know how to handle the error recovery though... I'd suggest adding a parser test for the x.0. error recovery though, even if we don't handle it very well currently.

@robojumper
Copy link
Contributor Author

I'd suggest adding a parser test for the x.0. error recovery though

Done. It had to be a failure test because the validation logic I added rejects attempts to index tuples with anything other than decimal integers without underscores or suffixes, as per the Rust reference.

@matklad
Copy link
Member

matklad commented Apr 5, 2019

I think we should avoid intorducing a TupleIndex node, I think it won't be generally useful.

Instead, I suggest hand-writing an

fn index_token(&self) -> Option<SyntaxToken>

method manually, which looks for token among children_with_tokens

We might also add an

enum FieldKind {
    Name(NameRef),
    Index(SyntaxToken)
}

and corresponding accessor for convenience.

@matklad
Copy link
Member

matklad commented Apr 5, 2019

The second commit verifies that this doesn't fix autocompletion because we still parse a float literal

Fascinating! So what happens is that, when we insert a fake identifier, we parse self.0.fakeIdent correctly. However, when we than try to lookup the receiver in the original file, we parse self.0. as self, . and 0., which is not unreasonable.

I guess an immediate thing we can do is to rewrite the completion test such that there's at least one char after dot, likeself.0.a. Fixing general tuple fields handling seems more important than fixing this particular error recovery quirk.

As for fixing the quirk.... I honestly don't immediately see a clean solution here. We can tweak the lexer to not lex 0. as a floating point literal, if the preceding token is ., but looking at the previous token changes the mode the lexer operates in significantly. I would game for maybe remapping the token from FP literal to an int and a dot at the parser level? This would still be a horrible hack, but at least it would be at the level of abstraction where looking at the context is ok?

@robojumper
Copy link
Contributor Author

@matklad I have incorporated the feedback from your comments -- ast::FieldKind is now there to facilitate inference and validation. I have also modified the test to verify that tuple struct dot completion generally works, and left a bunch of FIXMEs in the code that need to be touched if one were to hack around the lexer behavior.

@matklad
Copy link
Member

matklad commented Apr 5, 2019

bors r+

bors bot added a commit that referenced this pull request Apr 5, 2019
1117: [WIP] Tuple struct index inference r=matklad a=robojumper

The first commit adds a helper struct `ast::FieldKind` to facilitate inference.

The second commit adds a slightly modified test from #1109 while mentioning that there is a problem with how we're handling tuple indexing / floats.

cc #1109 

Co-authored-by: robojumper <[email protected]>
@robojumper robojumper changed the title [WIP] Tuple struct index inference Tuple struct index inference Apr 5, 2019
@bors
Copy link
Contributor

bors bot commented Apr 5, 2019

Build succeeded

@bors bors bot merged commit 2caa690 into rust-lang:master Apr 5, 2019
@matklad
Copy link
Member

matklad commented Apr 5, 2019

Ah, remapping tokens on the parser level won't be trivial because parser intentionally doesn't know about the token's text...

@flodiebold
Copy link
Member

Hm, couldn't we just handle this during lowering to HIR? I think it should just work if we removed the. there...

@matklad
Copy link
Member

matklad commented Apr 6, 2019

We'll need to tweak both HIR and completion_ctx. SPecifically, this line in context is currently failing: https://github.com/rust-analyzer/rust-analyzer/blob/990e74ba7c77485f914434ac6f09a40d1364634d/crates/ra_ide_api/src/completion/completion_context.rs#L214

Thinking about it more, I don't like the idea of hacking either lexer (b/c that adds context to lexer) or parser (b/c it adds back knowledge about token texts to parser), so I am indeed inclined towards patching this (in a pretty horrible way, by looking at the sibling token of field expressions) when converting to HIR / completing.

@robojumper robojumper deleted the 1109-my-tuples-need-me branch April 6, 2019 08:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants