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

Issue 519 method arg check #527

Merged
merged 6 commits into from
Dec 16, 2023
Merged

Issue 519 method arg check #527

merged 6 commits into from
Dec 16, 2023

Conversation

yamakoud
Copy link
Contributor

@yamakoud yamakoud commented Dec 13, 2023 •
edited
Loading

close #519

What I did:
Add functionality to raise argument error for extraneous arguments

yamakoud
yamakoud commented Dec 13, 2023
View reviewed changes
lib/skc_ast2hir/src/convert_exprs/method_call.rs Outdated
@@ -44,6 +44,10 @@ pub fn convert_method_call(
let found = mk
.class_dict
.lookup_method(receiver_ty, method_name, locs)?;

let total_args = args.unnamed.len() + args.named.len();
validate_argument_length(total_args, &found.sig.params)?;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[IMO]
It's good to validate argument length before types checking.

yamakoud
yamakoud commented Dec 13, 2023
View reviewed changes
tests/snapshots/erroneous__erroneous@method_call__fewer_argument_error.sk.snap Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[comment]

This PR makes shiika can also catch fewer argument error.
It's not in scope of this issue and if you don't want to catch fewer argument error, please notify me.

I'll change this PR to contain changes for make shiika catches extra argument error only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ahhh, this breaks ci process

image

Copy link
Contributor Author

@yamakoud yamakoud Dec 13, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I've reverted it

yamakoud
yamakoud commented Dec 13, 2023
View reviewed changes
Copy link
Contributor Author

@yamakoud yamakoud left a comment

Choose a reason for hiding this comment

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

self reviewed

yamakoud
yamakoud commented Dec 13, 2023
View reviewed changes
lib/skc_ast2hir/src/convert_exprs/method_call.rs
@@ -162,6 +166,18 @@ pub fn arrange_named_args<'a>(
Ok(v)
}

/// Check if number of arguments matches to the params.
fn validate_argument_length(total_args: usize, params: &[MethodParam]) -> Result<()> {
if total_args > params.len() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[NOTE]
I want to validate fewer argument also but after make this comparison to total_args != params.len(), the change breaks tests.

I'll work for validating fewer argument error in next issue if you like it.
In that case, please notify me and assign me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Screen Shot
image

Copy link
Collaborator

Choose a reason for hiding this comment

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

I want to validate fewer argument also but after make this comparison to total_args != params.len(), the change breaks tests.

Yes, that's because a parameter can have a default value.

class A
  def self.foo(a: Int, b = 2); end
end
A.foo(1)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhhhh I got it.

yamakoud
yamakoud commented Dec 13, 2023
View reviewed changes
Copy link
Contributor Author

@yamakoud yamakoud left a comment

Choose a reason for hiding this comment

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

self reviewed

@yamakoud yamakoud marked this pull request as ready for review December 13, 2023 04:12
@yamakoud
Copy link
Contributor Author

yamakoud commented Dec 13, 2023 •
edited
Loading

@yhara
This PR is ready for review.

Please review this whenever it's convenient for you.

@yhara
Copy link
Collaborator

yhara commented Dec 13, 2023

Thank you for your PR! Implementation is already perfect but how about adding a error report?
With a error report argument_error will look like this:

pub fn argument_error(
    expected: usize,
    got: usize,
    locs: LocationSpan,
) -> anyhow::Error {
    let main_msg = format!("wrong number of arguments: expected {}, got {}", expected, got);
    let sub_msg = format!("expected {}, got {}", expected, got);
    let report = skc_error::report_builder() 
        .annotate(locs, sub_msg)
        .build(main_msg, &locs);
    type_error(report)
}

@yamakoud
Copy link
Contributor Author

how about adding a error report?

I'll try that in a few days!
Thank you for the information.

yamakoud
yamakoud commented Dec 14, 2023
View reviewed changes
Copy link
Contributor Author

@yamakoud yamakoud left a comment

Choose a reason for hiding this comment

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

self reviewed

@yamakoud yamakoud requested a review from yhara December 14, 2023 13:48
@yhara yhara merged commit c7fc8c7 into shiika-lang:main Dec 16, 2023
2 checks passed
@yhara
Copy link
Collaborator

yhara commented Dec 16, 2023

Merged. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yhara yhara Awaiting requested review from yhara

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

Successfully merging this pull request may close these issues.

Should raise error for extranous arg
2 participants
@yamakoud @yhara