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

#2976. Add patterns tests #3031

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Conversation

sgrekhov
Copy link
Contributor

No description provided.

Copy link
Member

@eernstg eernstg left a comment

Choose a reason for hiding this comment

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

Looks good, with a couple of issues.

main() {
String res = "";
switch (C(1)) {
case .one!: // ignore: unnecessary_null_assert_pattern
Copy link
Member

Choose a reason for hiding this comment

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

That wouldn't be allowed after all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Deleted this test.


main() {
String res = "";
if (C(2) case > (.one as C)) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't think .one can be resolved, because as, even as a pattern, doesn't provide a useful context type.

Copy link
Member

Choose a reason for hiding this comment

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

Similarly for the other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted this test


main() {
String res = "";
if (C(1) case .one?) { // ignore: unnecessary_null_check_pattern
Copy link
Member

Choose a reason for hiding this comment

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

Do we really get unnecessary_null_check_pattern when the static type has a ?? If this is true then we should probably report it as an issue on the analyzer. Warnings are not language-defined, so we may not have any good source of truth about this question, but it seems reasonable to get it clarified.

Copy link
Contributor Author

Choose a reason for hiding this comment

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


main() {
String res = "";
if (C(1) case C.one!) { // ignore: unnecessary_null_assert_pattern
Copy link
Member

Choose a reason for hiding this comment

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

Similar issue, I'm surprised that the warning doesn't get emitted based on the static type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same as the above


main() {
String res = "";
if (C(1) case (C.one)) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps?:

Suggested change
if (C(1) case (C.one)) {
if (C(1) case (.one)) {

Similarly for the other cases.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I'm writing these tests with the full syntax to make sure that everything is working, and then I remove the types at the end. Sometimes I forget to do the last step, sorry.

/// other than for `==` and `!=`, will have the parameter type of the
/// corresponding operator of the matched value type as context type.
///
/// @description Checks that if a static member shorthand expression occurs in a
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// @description Checks that if a static member shorthand expression occurs in a
/// @description Checks that if a static member shorthand expression occurs in an

}
Expect.equals("ET Ok", res);

if ((x: ET(1)) case (x: .one,)) {
Copy link
Member

Choose a reason for hiding this comment

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

Perhaps?:

Suggested change
if ((x: ET(1)) case (x: .one,)) {
if ((x: ET(1)) case (x: .one)) {

A record containing one named field doesn't need the trailing comma. It is allowed (just checked), but I think it's OK to use the typical syntax here (where we're not testing record syntax).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanx. Removed.

}
Expect.equals("C Ok", res);

if (c case Container(m: .one,)) {
Copy link
Member

Choose a reason for hiding this comment

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

Same thing here: I think this test can omit the trailing comma. (We should have some other test about patterns on their own, independently of static access shorthands, where it is checked that these trailing commas don't crash the parser).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Fixed here and below.

Copy link
Contributor Author

@sgrekhov sgrekhov left a comment

Choose a reason for hiding this comment

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

Updated. PTAL.

main() {
String res = "";
switch (C(1)) {
case .one!: // ignore: unnecessary_null_assert_pattern
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Deleted this test.


main() {
String res = "";
if (C(2) case > (.one as C)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted this test


main() {
String res = "";
if (C(1) case .one?) { // ignore: unnecessary_null_check_pattern
Copy link
Contributor Author

Choose a reason for hiding this comment

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


main() {
String res = "";
if (C(1) case C.one!) { // ignore: unnecessary_null_assert_pattern
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The same as the above


main() {
String res = "";
if (C(1) case (C.one)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I'm writing these tests with the full syntax to make sure that everything is working, and then I remove the types at the end. Sometimes I forget to do the last step, sorry.

}
Expect.equals("ET Ok", res);

if ((x: ET(1)) case (x: .one,)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanx. Removed.

}
Expect.equals("C Ok", res);

if (c case Container(m: .one,)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. Fixed here and below.

@sgrekhov sgrekhov requested a review from eernstg January 9, 2025 10:35
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