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.

Sign up for GitHub

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

Jump to bottom

Decouple parse & resolve steps #148

Merged
merged 58 commits into from
Sep 14, 2022
Merged

Decouple parse & resolve steps #148

merged 58 commits into from
Sep 14, 2022

Conversation

xonixx
Copy link
Contributor

@xonixx xonixx commented Aug 30, 2022

This PR is prepared in context of #144.

It implements the part

However, that would require separating out the resolver to a separate pass ... which is probably good to do anyway, but a bit of work.

Since the change is reasonably big I would add couple clarifications.

  1. I tried to do only minimal work to get the task in question done, though I could not resist some (hopefully) minor refactorings and restructuring.
  2. It appears that resolving step needs some positions information, but not for all nodes. So at this point to keep the things simple and limited I wired the position only through very few of AST nodes. If need be we can equip all AST nodes with positions in generic way later but no need at this point.
  3. I decided that we don't need to move ALL things that were in resolve.go to separate step. To me some things logically belong more to parsing than resolution. Example: multiExprs thing.
  4. Since the parsing & resolving needed the same approach to errors with positions I generified this a bit by introducing the PositionError which is now panic-thrown internally by both parser & resolver. However I kept the external parser interface intact to return documented ParseError.
  5. I checked the test suite - seems to work fine.

xonixx added 30 commits August 25, 2022 17:35
@xonixx
decoupling parse/resolve
6d42e3d
@xonixx
decoupling parse/resolve
62c0e91
@xonixx
decoupling parse/resolve
6f97aea
@xonixx
decoupling parse/resolve
a2bb302
@xonixx
decoupling parse/resolve
4a93430
@xonixx
decoupling parse/resolve : functions
f4b7a25
@xonixx
decoupling parse/resolve : visitor approach from golang
fac460d
@xonixx
decoupling parse/resolve
55b69ad
@xonixx
decoupling parse/resolve : visitor approach from golang
bab57ba
@xonixx
decoupling parse/resolve
da3f570
@xonixx
decoupling parse/resolve : multiExpr
bf2a0ea
@xonixx
decoupling parse/resolve : rfct
4214783
@xonixx
decoupling parse/resolve : rfct
04e70d7
@xonixx
decoupling parse/resolve : rfct
6ff111c
@xonixx
decoupling parse/resolve : work on sufficient position wiring for res…
d1d5edf 
…olve
@xonixx
decoupling parse/resolve : varRef/arrayRef
945e48e
@xonixx
decoupling parse/resolve : varRef/arrayRef
6727e86
@xonixx
decoupling parse/resolve
d668547
@xonixx
decoupling parse/resolve
2b6f2e6
@xonixx
decoupling parse/resolve : fight with import cycles
2e26c0e
@xonixx
decoupling parse/resolve : fight with import cycles
b63e992
@xonixx
decoupling parse/resolve : fight with import cycles
3219855
@xonixx
decoupling parse/resolve : fixing failing tests
25d2860
@xonixx
decoupling parse/resolve : fixing failing tests
fdd96be
@xonixx
decoupling parse/resolve : fixing failing tests
9399ab1
@xonixx
decoupling parse/resolve : fixing failing tests
d801d4b
@xonixx
decoupling parse/resolve : fixing failing tests
2fc4749
@xonixx
decoupling parse/resolve : fixing failing tests
e7279c4
@xonixx
decoupling parse/resolve : fixing failing tests
ecbdeea
@xonixx
decoupling parse/resolve : rfct
6078dd5
@xonixx
Merge branch 'master' into decouple_parse_resolve
0b1d67d 
# Conflicts:
#	parser/parser.go
@xonixx
Copy link
Contributor Author

xonixx commented Aug 31, 2022

Note that I just merged another change, so there's a small conflict in parser.go which needs fixing.

Merged master.

@xonixx
Copy link
Contributor Author

xonixx commented Sep 8, 2022

@benhoyt Any update?

@benhoyt
Copy link
Owner

benhoyt commented Sep 8, 2022 via email

@xonixx
Copy link
Contributor Author

xonixx commented Sep 8, 2022

Sure no problem. Have a pleasant trip!

benhoyt
benhoyt requested changes Sep 11, 2022
View reviewed changes
Copy link
Owner

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Thanks for your work here. I think it's really good work overall. I've left a bunch of comments, mostly minor and hopefully easy enough to address.

internal/ast/ast.go Outdated Show resolved Hide resolved
internal/ast/ast.go Outdated Show resolved Hide resolved
internal/ast/walk.go Show resolved Hide resolved
internal/ast/walk.go Outdated Show resolved Hide resolved
internal/ast/walk.go Outdated Show resolved Hide resolved
parser/parser.go Outdated Show resolved Hide resolved
parser/parser.go Outdated Show resolved Hide resolved
parser/parser.go Show resolved Hide resolved
internal/resolver/resolve.go Outdated Show resolved Hide resolved
parser/parser.go Show resolved Hide resolved
xonixx added 12 commits September 13, 2022 00:06
@xonixx
decoupling parse/resolve : CR : adding node() to Node interface
787ffd3
@xonixx
decoupling parse/resolve : CR : renaming
7faa3a8
@xonixx
decoupling parse/resolve : CR : adjust comment for Walk function
478b645
@xonixx
decoupling parse/resolve : CR : WillBeResolvedLater -> resolvedLater
5a58be1
@xonixx
decoupling parse/resolve : CR : cleanup comment
2c07a7a
@xonixx
decoupling parse/resolve : CR : Let's change parser.function() to r…
b039569 
…eturn a `*ast.Function` instead, to make things consistent. The same for `Action`
@xonixx
decoupling parse/resolve : CR : removing EndPos since was used in s…
ee75278 
…ingle place that is not that important
@xonixx
decoupling parse/resolve : CR : remove Position.Add() and adjust er…
7cd38cc 
…ror msgs in tests
@xonixx
decoupling parse/resolve : CR : move PositionError to ast
8b65e32
@xonixx
decoupling parse/resolve : CR : rfct
7b6e450
@xonixx
decoupling parse/resolve : CR : restore fields back to ParseError
8794caa
@xonixx
decoupling parse/resolve : CR : restore fields back to ParseError
3be851f
@xonixx
Copy link
Contributor Author

xonixx commented Sep 13, 2022

Hey @benhoyt! Hopefully, I've resolved all items except for the one with WalkExprList/WalkStmtList - please see my comment there. What do you think?

benhoyt
benhoyt approved these changes Sep 13, 2022
View reviewed changes
Copy link
Owner

@benhoyt benhoyt left a comment

Choose a reason for hiding this comment

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

Great! Let's just fix these few comment nits and I'll squash and merge this!

internal/ast/ast.go Show resolved Hide resolved
internal/ast/ast.go Show resolved Hide resolved
internal/ast/ast.go Show resolved Hide resolved
internal/ast/ast.go Outdated Show resolved Hide resolved
internal/ast/walk.go Outdated Show resolved Hide resolved
internal/resolver/resolve.go Outdated Show resolved Hide resolved
parser/parser.go Show resolved Hide resolved
@xonixx
Copy link
Contributor Author

xonixx commented Sep 13, 2022

Done.

@benhoyt benhoyt merged commit 0a84980 into benhoyt:master Sep 14, 2022
@benhoyt
Copy link
Owner

benhoyt commented Sep 14, 2022

Thanks, merged!

@xonixx xonixx mentioned this pull request Oct 6, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benhoyt benhoyt benhoyt approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@xonixx @benhoyt