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

Disallow named arguments for methods of a type union where parameter names differ #2194

Merged
merged 2 commits into from
Sep 1, 2017

Conversation

chalcolith
Copy link
Member

@chalcolith chalcolith commented Aug 26, 2017

Working on learning more about the compiler...

This fixes the bug noted in #394, where named parameters can be used with methods in a union type whose parameter names differ.

The change makes it a compile-time error to call a method on a union type using named arguments when the methods in the union types have differently-named parameters.

Was:
This fixes the bug noted in #394, where named parameters can be used with
methods in a union type whose parameter names differ.

The change makes it a compile-time error to call a method on a union type
(whether using named parameters or not) when the methods in the union types
have differently-named parameters.

{
ast_free_unattached(r);
ok = false;
}
Copy link
Member

Choose a reason for hiding this comment

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

our else block handling is wildly inconsistent throughout the codebase.

we should put together a c coding style

@jemc
Copy link
Member

jemc commented Aug 26, 2017

whether using named parameters or not

I think this is overly aggressive. Note that the rules discussed in the referenced ticket were:

  • When calling on a union without passing arguments by name the names don't matter.
  • When calling on a union passing any arguments by name ALL parameter names must match on all types in the union.

@chalcolith
Copy link
Member Author

chalcolith commented Aug 26, 2017

From what I can tell the function lookup happens before we know that positional or named parameters are in use. Is there a way to annotate the function type AST node so that when the function call is processed we can reject it if the names are different?

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label Aug 30, 2017
… when using named arguments.

This fixes the bug noted in ponylang#394, where named parameters can be used with methods in a union type whose parameter names differ.

The change makes it a compile-time error to call a method on a union type using named arguments when the methods in the union types have differently-named parameters.
@chalcolith chalcolith force-pushed the fix_394_union_method_args branch from b61d93e to 9bee8c2 Compare August 30, 2017 19:45
@chalcolith
Copy link
Member Author

I have added a check for the use of named arguments before triggering a compiler error. I am not very confident given my limited knowledge of the compiler internals that this will catch all the necessary cases.

@chalcolith chalcolith added changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge and removed do not merge This PR should not be merged at this time labels Sep 1, 2017
@chalcolith chalcolith requested a review from jemc September 1, 2017 16:13
@jemc jemc changed the title Disallow calling methods of a type union whose parameter names differ (#394) Disallow named arguments for methods of a type union where parameter names differ Sep 1, 2017
@jemc jemc merged commit 8e5b520 into ponylang:master Sep 1, 2017
ponylang-main added a commit that referenced this pull request Sep 1, 2017
@chalcolith chalcolith deleted the fix_394_union_method_args branch September 7, 2017 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - fixed Automatically add "Fixed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants