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

Improve error message for match on structs #3746

Merged
merged 1 commit into from
May 3, 2021
Merged

Conversation

ergl
Copy link
Member

@ergl ergl commented Apr 29, 2021

Since a struct doesn't have a type descriptor, it's impossible to match
against it, unless it's exactly the same struct type. Before, the
return type of matchtype.c:is_matchtype didn't convey enough information
to the caller about the reason why two types couldn't match. In most
cases, this is due to reference capabilities, so that's what the error
message shows.

By distinguishing the specific case of a match failure due to lack of
type descriptor, the compiler can give a friendlier error message to the
user.


This fixes #2000 and fixes #3565, and mostly follows the approach described by @jemc on the second issue.

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.

Nice work!

src/libponyc/type/matchtype.c Outdated Show resolved Hide resolved
@SeanTAllen
Copy link
Member

@ergl should this be merged?

also, do you think this deserves a changelog entry and release notes?

@ergl
Copy link
Member Author

ergl commented May 1, 2021

@SeanTAllen Yes, I think it's ready to be merged. As for release notes, sound good, this is probably worth mentioning.

@ergl ergl added the changelog - changed Automatically add "Changed" CHANGELOG entry on merge label May 1, 2021
@ponylang-main
Copy link
Contributor

Hi @ergl,

The changelog - changed label was added to this pull request; all PRs with a changelog label need to have release notes included as part of the PR. If you haven't added release notes already, please do.

Release notes are added by creating a uniquely named file in the .release-notes directory. We suggest you call the file 3746.md to match the number of this pull request.

The basic format of the release notes (using markdown) should be:

## Title

End user description of changes, why it's important,
problems it solves etc.

If a breaking change, make sure to include 1 or more
examples what code would look like prior to this change
and how to update it to work after this change.

Thanks.

@SeanTAllen
Copy link
Member

This commit can be merged once @ergl adds release notes. I'm adding a "do not merge" label until then.

@SeanTAllen SeanTAllen added the do not merge This PR should not be merged at this time label May 1, 2021
@SeanTAllen
Copy link
Member

@ergl this has a merge conflict that will need to be resolved in addition to the addition of release notes.

Since a struct doesn't have a type descriptor, it's impossible to match
against it, unless it's exactly the same struct type. Before, the
return type of matchtype.c:is_matchtype didn't convey enough information
to the caller about the reason why two types couldn't match. In most
cases, this is due to reference capabilities, so that's what the error
message shows.

By distinguishing the specific case of a match failure due to lack of
type descriptor, the compiler can give a friendlier error message to the
user.
@ergl ergl force-pushed the ergl/fix_3565 branch from 97c632d to bcb1a7e Compare May 3, 2021 14:31
@ergl
Copy link
Member Author

ergl commented May 3, 2021

Rebased and added release notes. I've added an example of the new error message in the release notes, but perhaps that's too much? I'm willing to trim it back to just the first paragraph.

@SeanTAllen
Copy link
Member

@ergl looks good to me

@SeanTAllen SeanTAllen removed the do not merge This PR should not be merged at this time label May 3, 2021
@SeanTAllen SeanTAllen merged commit 7369194 into main May 3, 2021
@SeanTAllen SeanTAllen deleted the ergl/fix_3565 branch May 3, 2021 15:57
github-actions bot pushed a commit that referenced this pull request May 3, 2021
github-actions bot pushed a commit that referenced this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog - changed Automatically add "Changed" CHANGELOG entry on merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Pattern match on struct violates capabilities Bad error message when matching on a struct in a union
4 participants