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

Add default location to parameters node #959

Merged
merged 1 commit into from
Jun 8, 2023

Conversation

jemmaissroff
Copy link
Collaborator

This is currently breaking on 32 bit machines because in the case where we only have errors in the parameters, we never set the location, and then have underflow if the file is sufficiently large

This is currently breaking on 32 bit machines because in the case
where we only have errors in the parameters, we never set the
location, and then have underflow if the file is sufficiently large
Copy link
Collaborator

@kddnewton kddnewton left a comment

Choose a reason for hiding this comment

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

I'm approving this, but I think we shouldn't be creating parameters nodes in the case that they're entirely empty. Could you find out in where in the tree we're creating these extra nodes?

@@ -2458,7 +2458,7 @@ yp_parameters_node_create(yp_parser_t *parser) {
*node = (yp_parameters_node_t) {
{
.type = YP_NODE_PARAMETERS_NODE,
.location = { .start = NULL, .end = NULL },
.location = { .start = parser->current.start, .end = parser->current.start },
Copy link
Member

Choose a reason for hiding this comment

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

Should it be YP_LOCATION_NULL_VALUE maybe?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think in this case we want it to be where the current token is, because that's where the parameters node is too!

@jemmaissroff
Copy link
Collaborator Author

I'm approving this, but I think we shouldn't be creating parameters nodes in the case that they're entirely empty. Could you find out in where in the tree we're creating these extra nodes?

Not sure I'm following this question. We create the parameters nodes here before we've actually parsed the parameters. We then go through and parse parameters, and if they're all invalid / errors, we'll still have the parameters node.

This is a snippet from our errors test which triggered the code path where we create a parameters node with an invalid location: "def foo(A, @a, $A, @@a);end"
and here's the parsed tree (prior to this commit):

#<YARP::ParseResult:0x000000010733b140
 @comments=[],
 @errors=
  [#<YARP::ParseError:0x0000000107474750 @location=(8...17), @message="Formal argument cannot be a constant">,
   #<YARP::ParseError:0x00000001074746d8 @location=(11...24), @message="Formal argument cannot be an instance variable">,
   #<YARP::ParseError:0x0000000107474660 @location=(13...27), @message="Unexpected ','.">,
   #<YARP::ParseError:0x0000000107474610 @location=(14...28), @message="Expected ')' after left parenthesis.">,
   #<YARP::ParseError:0x00000001074745c0 @location=(14...28), @message="Expected to be able to parse an expression.">,
   #<YARP::ParseError:0x0000000107474570 @location=(20...40), @message="Expected a newline or semicolon after statement.">,
   #<YARP::ParseError:0x0000000107474520 @location=(20...40), @message="Expected to be able to parse an expression.">],
 @value=
  ProgramNode(0...25)(
    [],
    StatementsNode(0...25)(
      [DefNode(0...25)(
         (4...7),
         nil,
         ParametersNode(-4391280584...-4391280584)([], [], [], nil, [], nil, nil),
         StatementsNode(14...20)([MissingNode(14...14)(), ClassVariableReadNode(17...20)(), MissingNode(20...20)()]),
         [],
         (0...3),
         nil,
         (7...8),
         (14...14),
         nil,
         (22...25)
       )]
    )
  ),
 @warnings=[]>

@kddnewton
Copy link
Collaborator

Yeah I'm saying we shouldn't be calling parse_parameters in places where we know the next token can't possibly start a set of parameters.

@jemmaissroff
Copy link
Collaborator Author

Okay, merging now, but created #961 to fix this.

@jemmaissroff jemmaissroff merged commit 2265dd1 into main Jun 8, 2023
@kddnewton kddnewton deleted the add-default-location-to-params-node branch August 15, 2023 18:24
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