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

parser: Error on function redeclaration #69

Merged
merged 1 commit into from
Feb 8, 2023
Merged

parser: Error on function redeclaration #69

merged 1 commit into from
Feb 8, 2023

Conversation

juliaogris
Copy link
Member

Error on function redeclaration, including builtin functions. Before we
would just let new functions override existing functions which is
pretty confusing, err early err often.

@github-actions
Copy link

github-actions bot commented Feb 8, 2023

firebase-deployment: https://evy-lang--69-zhgobwkg.web.app (dfc06ad)

@juliaogris juliaogris requested a review from camh- February 8, 2023 10:40
Copy link
Contributor

@camh- camh- left a comment

Choose a reason for hiding this comment

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

🥬 LGTM

p := &Parser{funcs: builtins, wssStack: []bool{false}}

p := &Parser{funcs: map[string]*FuncDecl{}, wssStack: []bool{false}}
for name, funcDecl := range builtins {
Copy link
Contributor

Choose a reason for hiding this comment

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

🍍 shame you can't use generics - you could use x/exp/maps.Clone() here (or on the line before even: funcs: maps.Clone(builtins))

Copy link
Member Author

Choose a reason for hiding this comment

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

oh nice!! i think tinygo 1.27.0 is coming out soon and it works with got 1.20.
I wonder if the issue was again that I didn't hoist things on main.
I've got a cleanup PR due that replaces all errormessages with Printfs now that that's possible. In fact, I should maybe consider exposing them in evy!?

@@ -80,6 +84,14 @@ func New(input string, builtins map[string]*FuncDecl) *Parser {
p.advanceTo(i)
fd := p.parseFuncDeclSignature()
if fd != nil {
if p.funcs[fd.Name] != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps a switch?:

switch {
case p.funcs[fd.Name] == nil:
  p.funcs[fd.Name] = fd
case builtins[fd.Name] == nil:
  p.appendErrorForToken("redeclaration of function "+fd.Name, fd.Token)
case builtins[fd.Name] != nil: // could be `default:`, but maybe this reads better?
  p.appendErrorForToken("cannot override builtin function "+fd.Name, fd.Token)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

or perhaps swap the last two cases?

Copy link
Member Author

Choose a reason for hiding this comment

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

ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

switch {
case  p.funcs[fd.Name] != nil && builtins[fd.Name] == nil:
  p.appendErrorForToken("redeclaration of function "+fd.Name, fd.Token)
case p.funcs[fd.Name] != nil && builtins[fd.Name] != nil: 
  p.appendErrorForToken("cannot override builtin function "+fd.Name, fd.Token)
default:
  p.funcs[fd.Name] = fd
}

Copy link
Member Author

Choose a reason for hiding this comment

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

any of them. i'll got with your suggestion.

Copy link
Contributor

Choose a reason for hiding this comment

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

I deliberately didn't do it your way since you can take advantage of the case ordering to put the happy path first and not require the more complicated case expressions. but up to you

Copy link
Member Author

Choose a reason for hiding this comment

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

but we don't do the happy path first typically.
we do it left and last, don't we?

Copy link
Contributor

Choose a reason for hiding this comment

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

but rules are made to be broken. happy path is more about less nesting than what comes first. it just ends up putting the happy path last mostly.

Error on function redeclaration, including builtin functions. Before we
would just let new functions override existing functions which is
pretty confusing, err early err often.
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.

None yet

2 participants