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 some tests exhibiting meaning of generic type constraints #18213

Merged
merged 2 commits into from
Aug 13, 2021

Conversation

bradcray
Copy link
Member

@bradcray bradcray commented Aug 12, 2021

This captures some patterns we were discussing today regarding the
interpretation of R in the event that a generic record is fully, partially,
or not at all defaulted. I hadn't internalized these rules correctly, so
wrote this both to improve my understanding and ensure we had a test
that locked in the expected behavior (though we likely do, but I have a
"the more tests the better" mindset).

This captures some patterns we were discussing today that I hadn't
internalized correctly.  Hopefully writing these will help me keep the
rules straight and lock in the current behavior if we don't already
have tests covering them (we very well may).

---
Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
@bradcray
Copy link
Member Author

@e-kayrakli : Following up on our discussion and my confusion this morning, would you review these to see if they match what we were discussing and whether you think there are other key patterns to try and capture / lock in? (I put this together while writing up an "are we sure this is right?" issue).

Copy link
Contributor

@e-kayrakli e-kayrakli left a comment

Choose a reason for hiding this comment

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

I can't think of any other case at the moment. Looks good!

FWIW, nothing here made me think whether we should change where we are today.

@@ -0,0 +1,3 @@
allGenericsDefaulted.chpl:43: error: unresolved call 'baz(R(real(64),2))'
allGenericsDefaulted.chpl:17: note: this candidate did not match: baz(r: R(int, ?rank))
allGenericsDefaulted.chpl:17: note: because an argument was incompatible
Copy link
Contributor

Choose a reason for hiding this comment

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

Clearly not about this PR, but I am not sure if should try to make the error messages in {1,2} to be similar to {3,4}. The latter set seems more vague, and it can be less helpful if the function had many arguments.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't disagree, but haven't felt sufficiently inspired to file the issue (yet). Let me know if you think it's important that I do so (vs. waiting for someone to truly be annoyed enough by it to get around to it themselves).

Copy link
Contributor

Choose a reason for hiding this comment

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

Nope. This error message definitely falls into "good enough for now" category for me.

@bradcray
Copy link
Member Author

One thought I had given the exchange we just had on #18215 would be to expand the partially and completely generic cases to see what happens to the variable declarations if R is used with an initializer. Might give that a try before merging.

Happily, they are symmetrical with formal types.

---
Signed-off-by: Brad Chamberlain <bradcray@users.noreply.github.com>
@bradcray
Copy link
Member Author

@e-kayrakli : As alluded to yesterday, I did end up adding another line to each test to explore what a declaration of the form var r: R = new R(); would do where the initializing expression didn't match the defaults of R and they acted the same as the formal arguments, as hoped. I don't think they require an additional review from you, but wanted to make you aware in case you were curious.

@e-kayrakli
Copy link
Contributor

Thanks!

@bradcray bradcray merged commit c0e3512 into chapel-lang:main Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants