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 stricter checks for arguments to macro methods on AST nodes #10498

Merged

Conversation

HertzDevil
Copy link
Contributor

Currently, Crystal's macro interpreter is very lax when it comes to macro methods on AST nodes:

{% [1, 2].includes?(1, a: "x") { |b, c| } %} # named args and block silently allowed
{% [1].uniq(2, 3, 4) %}                      # some methods don't check their args at all

Other times, the error messages related to parameter-argument matching are inconsistent with other places in the compiler:

# other places use "wrong number of arguments..." instead
{% [1].reduce(2, 3) { |x, y| } %} # Error: only 0 or 1 args expected for reduce, got 2

# normal code uses "... is expected to be invoked with a block, but no block was given"
{% [1].reduce %} # Error: reduce expects a block

# should be "expected 1..2" since `ArrayLiteral#[](index, count)` is a valid overload
{% [1][0, 3, 6] %} # Error: wrong number of arguments for Crystal::ArrayLiteral#[] (given 3, expected 1)

This PR fixes all of these with a new interpret_check_args macro, replacing the old interpret_argless_method and friends; this shouldn't break any code that already obeys the API docs on src/compiler/crystal/macros.cr.

It's best to review each commit in isolation instead of all at once. Error messages for AST node type mismatches (argument must be a T, not U) will be covered later.

Copy link
Member

@beta-ziliani beta-ziliani left a comment

Choose a reason for hiding this comment

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

🚀

@beta-ziliani beta-ziliani added this to the 1.2.0 milestone Aug 20, 2021
@straight-shoota straight-shoota merged commit db4a26b into crystal-lang:master Aug 24, 2021
@HertzDevil HertzDevil deleted the bug/macro-argument-checks branch August 25, 2021 01:08
@Sija Sija mentioned this pull request Aug 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants