Skip to content

fix: Fix parsing of nested tuple field accesses in a cursed way #14084

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

Merged
merged 8 commits into from
Feb 7, 2023

Conversation

Veykril
Copy link
Member

@Veykril Veykril commented Feb 3, 2023

This is absolutely terrible but seems to work. Macro fragment parsing comes next.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2023
@cynecx
Copy link
Contributor

cynecx commented Feb 4, 2023

Might be worth mentioning here I think, since I've implemented a similar but different approach (though still cursed) here: cynecx@3628848.
The basic idea is that the lexer produces three kind of FLOAT_NUMBER tokens: FLOAT_NUMBER (eg. -1.0), FLOAT_NUMBER_1 (eg. 1.) and FLOAT_NUMBER_2 (eg. 1.2). Then the parser can easily detect FLOAT_NUMBER_1 and FLOAT_NUMBER_2 and rewrite them, by injecting INT_NUMBER, DOT or INT_NUMBER, DOT, INT_NUMBER token sequences . This started as a more ambitious approach to generally support "injecting" tokens, however right now it's really limited to splitting floats. Also it's quite limited to non-macro usage right now.

@Veykril
Copy link
Member Author

Veykril commented Feb 4, 2023

Ah right, I remember this now. At its core these two seem to be the same approach roughly, at least both look terrible :) Fixing up macros is somewhat annoying since you'll need to mutate the correct subtrees (aka we need something like a CursorMut ...).

I'm not sure if we need the hack for anything else though, so a general injection support might be too much? At least I am hopeful that the language doesn't give us more of this kind of ambiguity ...

@Veykril
Copy link
Member Author

Veykril commented Feb 7, 2023

Turns out fixing up the inputs for macro expression fragments is a major PITA, so I skipped that for the time being as we aren't regressing on that side, just staying the same. Aside from that, things should just work. Assuming CI passes I'll try to merge it tonight or tomorrow so we can get some nightly testing in.

@Veykril Veykril marked this pull request as ready for review February 7, 2023 16:59
@Veykril
Copy link
Member Author

Veykril commented Feb 7, 2023

Not too fond of the parser changes required to make this work, but it's not like we can do this without any awful hacks so....
let's see if this works out (I am a bit worried about parser recovery not playing nicewith this)
@bors r+

@bors
Copy link
Contributor

bors commented Feb 7, 2023

📌 Commit a756c9a has been approved by Veykril

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Feb 7, 2023

⌛ Testing commit a756c9a with merge 7f12344...

@bors
Copy link
Contributor

bors commented Feb 7, 2023

☀️ Test successful - checks-actions
Approved by: Veykril
Pushing 7f12344 to master...

@bors bors merged commit 7f12344 into rust-lang:master Feb 7, 2023
@Veykril Veykril deleted the float-parse branch February 8, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants