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

Compile error when comparing sugared constructors with 'is' or 'isnt' (#2024) #2494

Merged
merged 1 commit into from
Jan 18, 2018

Conversation

ehooper
Copy link
Contributor

@ehooper ehooper commented Jan 14, 2018

#2024
Compile error when using 'is' to compare sugared type constructors, including cases where they are embedded in tuples or seqs.

I didn't include array literals in these changes, but I think those could also be an issue. For example, [1; 2; 3] is [1; 2; 3] is false, which could cause the same kind of confusion.

@SeanTAllen
Copy link
Member

Error in Windows compilation:

c:\projects\ponyc\src\libponyc\pass\refer.c(935): error C2440: '=': cannot convert from 'void *' to 'ast_t *'
c:\projects\ponyc\src\libponyc\pass\refer.c(935): note: Conversion from 'void*' to pointer to non-'void' requires an explicit cast

type = ast_data(ast);
if(ast_id(type) != TK_PRIMITIVE)
{
ast_error(opt->check.errors, ast, "comparing a non-primitive type constructor will always fail");
Copy link
Member

Choose a reason for hiding this comment

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

This is a good error message except I think the folks who are most likely to trigger it might not realize they are doing "as" against a constructor even when they see the error.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point, changed it. Hopefully it's clearer now.

Copy link
Member

@Praetonus Praetonus left a comment

Choose a reason for hiding this comment

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

Looks great, thank you.

I've left a comment on the source changes.

@@ -1334,6 +1378,8 @@ ast_result_t pass_refer(ast_t** astp, pass_opt_t* options)
case TK_ERROR: r = refer_error(options, ast); break;
case TK_COMPILE_ERROR:
r = refer_compile_error(options, ast); break;
case TK_IS:
Copy link
Member

Choose a reason for hiding this comment

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

This should cover TK_ISNT as well.

@jemc
Copy link
Member

jemc commented Jan 17, 2018

When we add support for TK_ISNT, I'd also like to see a test cover the isnt case as well.

…#2024)

An identity comparison with a new object will always be false, so this
will catch mistaken 'is' comparisons without affecting legitimate uses.
@ehooper
Copy link
Contributor Author

ehooper commented Jan 17, 2018

Oops. Added the TK_ISNT case.

Copy link
Member

@jemc jemc left a comment

Choose a reason for hiding this comment

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

Good to merge when CI passes.

@jemc
Copy link
Member

jemc commented Jan 18, 2018

CI failure is just Travis macos glitching out. Going to merge this.

@jemc jemc added the changelog - added Automatically add "Added" CHANGELOG entry on merge label Jan 18, 2018
@jemc jemc changed the title Compile error when comparing sugared constructors with 'is' (#2024) Compile error when comparing sugared constructors with 'is' or 'isnt' (#2024) Jan 18, 2018
@jemc jemc merged commit f37e281 into ponylang:master Jan 18, 2018
ponylang-main added a commit that referenced this pull request Jan 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - added Automatically add "Added" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants