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

Provide better error message when assigning to tuple element #1563

Merged
merged 4 commits into from
Oct 16, 2020

Conversation

rdmello
Copy link
Contributor

@rdmello rdmello commented Oct 15, 2020

Description

This change addresses the second part of #1560 by adding an explicit error message when assigning to a tuple element:

./src/bpftrace -e 'i:s:1 { $x = (1, 2); $x.1 = 1; }'
stdin:1:22-30: ERROR: Tuples are immutable. Consider creating a new tuple and assigning it instead.
i:s:1 { $x = (1, 2); $x.1 = 1; }
                     ~~~~~~~~

I went with the approach of making this a parse-time error for simplicity. I ran all the unit tests and verified that the number of shift-reduce conflicts haven't changed after implementing this.

An alternative approach is to allow tuple assignment to parse successfully, and then make tuple assignment error later during semantic analysis. This would be a little more complex, but would also provide a better error message (since it can disambiguate between assigning to tuples vs. assigning to maps). Let me know if this approach is preferable.

Checklist
  • Language changes are updated in docs/reference_guide.md
  • User-visible and non-trivial changes updated in CHANGELOG.md
  • The new behaviour is covered by tests

@rdmello rdmello changed the title Tuple assignment error Provide better error message when assigning to tuple element Oct 15, 2020
Copy link
Member

@danobi danobi left a comment

Choose a reason for hiding this comment

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

LGTM, but can you add a couple more tests for other conditions?

eg (1,0).1 = 0, ((1,0),3).0.0 = 3, ((1,0),3).0 = (0,1). You can just check that parsing failed for these, no need to check error message.

@danobi
Copy link
Member

danobi commented Oct 15, 2020

Oh, also update the changelog, please.

@rdmello
Copy link
Contributor Author

rdmello commented Oct 16, 2020

@danobi Thank you for reviewing this...I added a few more tests and updated the changelog.

@danobi
Copy link
Member

danobi commented Oct 16, 2020

Thanks for the PR!

@danobi danobi merged commit 0106960 into bpftrace:master Oct 16, 2020
@rdmello rdmello deleted the tuple_assignment_error branch October 16, 2020 18:35
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