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

Solve issue with variable names like $in or $struct #748

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ffrank
Copy link
Contributor

@ffrank ffrank commented Mar 12, 2024

Make variables like $foo and $in and $this.smells.funny into single lexical tokens. This avoids lexing errors when the variable name without the dollar sign looks like a reserved word.

(Frankly, I believe this is the more correct way to do this, and if not the only way, it is probably the easiest.)

This is inelegant, but I stronly believe that allowing dollar signs and their
variable name suffixes to both be first class citizens, is a bad idea.
The dollar sign should initiate a token that is exempt from any other
processing.

Note that it would be possible to construct a regex similar to the one for
IDENTIFIER, that only matches valid variable names. But it will be complex.
It seems more maintainable to just consume all allowed characters, and then
check validity with some simple golang in the parser actions.

Fixes purpleidea#728
@ffrank
Copy link
Contributor Author

ffrank commented Mar 12, 2024

As an aside, I may have found a race in https://github.com/ffrank/mgmt/actions/runs/8247933356/job/22557152150

(I'm fairly sure I didn't introduce that.)

@purpleidea
Copy link
Owner

Neat! I wasn't sure this was possible while also not blocking some other parsing changes I have. I appreciate the patch, but I will have to delay merging it to make sure it doesn't conflict with some other parsing stuff I have pending. Of note, if you look at the git history, you'll see I had something similar in until I realized it was causing a conflict.

Lastly, there's a good chance I get rid of the $foo style naming entirely (not yet) and moving to just foo. TBD.

The race: yes there is one that I know about that needs fixing.

@ffrank
Copy link
Contributor Author

ffrank commented Apr 1, 2024

So I suppose you are referring to 9e7b7fb.

That was some funky stuff. The dichotomy of "identifier" vs. "dotted identifier" makes things a bit tricky. The logic you removed in 9e7b7fb was complicated indeed, and I can see some ambiguities for the parser resulting from this.

The proposed patch avoids some (all?) of this by allowing the lexer to consume every variable name as a single token, and provide the information that the parser needs to make proper decisions (i.e., is it a dotted or undotted name token).

@purpleidea
Copy link
Owner

The proposed patch avoids some (all?) of this by allowing the lexer to consume every variable name as a single token, and provide the information that the parser needs to make proper decisions (i.e., is it a dotted or undotted name token).

Yes this is very clever, thank you. I'm slightly worried that doing this might make it more difficult in the future to add parser features without being ambiguous. I will look through my WIP feature branches and have a look.

@ffrank
Copy link
Contributor Author

ffrank commented Apr 13, 2024

FWIW I think it is really really awkward to make the dots and particles of qualified names into first class citizens on the parser level.

This is currently a valid resource

print "it" {
        msg => fmt.printf("%d", $c . a),
}

as is this

print "it" {
        msg => fmt.printf("%d", $c

.

                                                a),
}

I argue that $c.a should always be a token, as should fmt.printf.

Yes even this is accepted right now (this patch only addresses the variable):

import "fmt"

class container {
        $a = 1
}
include container as c

print "it" {
        msg => fmt . printf("%d", $c . a),
} #              ^^^^^^

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.

2 participants