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.

Sign up for GitHub

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

Jump to bottom

Warn for variables/fields declared with generic type without `?` #22745

Merged

mppf merged 21 commits into chapel-lang:main from mppf:warn-generic-fields-vars4

Jul 27, 2023

Member

mppf commented Jul 13, 2023 •

edited

Loading

Resolves https://github.com/Cray/chapel-private/issues/5032

This PR adds a warning for these cases:

var x: someGenericType for fields and variables (need someGenericType(?))
proc foo(type t) { var x: t; } -- similarly to var x: someGenericType
var x: f(); where f() returns a generic type, for fields and variables

In the above, someGenericType includes domain or user records/classes that are generic. Note that PR #22697 added a warning for fields declared with type domain or fields of class type with generic management.

Reviewed by @vasslitvinov - thanks!

full comm=none testing

Future work

warn for class C : SomeGenericType -- PR Add a warning for generic superclasses that don't include (?) #22784

mppf marked this pull request as ready for review

July 18, 2023 17:53

mppf added 21 commits

July 18, 2023 14:49


          Draft to warn for generic fields/decls

e138657

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Improve handling for ? in type exprs

ea3de9c

Necessary for change to DefaultAssociativeDomRehashHelper in
upcoming commit.

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Add (?) to two field types in internal modules

bd68e8a

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Add use of generic type formal to test

0ae82b5

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Skip warning for integral etc, better detect fields

6f0c2db

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Fix missing & redundant warnings

4b321b9

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Update a few tests

c78c1b5

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Fix generic-field.chpl

d8c18e8

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Adjust normalizer to compute FLAG_MARKED_GENERIC

58f67f6

For test/classes/jade/owned/assign-op.chpl

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Don't warn for 'var x: record'

f2129dc

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Don't warn for generic out formals

257e31c

For test/functions/intents/out/out-generic-type-set-assign.chpl

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Avoid redundant field-with-generic-management warnings

f79e984

For test/types/records/generic/warn-field-generic-declared-type.chpl

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Update SortedMap to use a concrete type for _set

cb976c4

Makes the code clearer and avoids the new warning.

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Update tests per new warning

231b564

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Use concrete type for sendWaiters nd recvWaiters

d82a486

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Update tests for new warning

faa9c9b

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Suppress warning for fields of owned/shared

cbc8a71

For test/types/partial/bad-managed-init.chpl

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Fix more tests for the new warning

d3a3fef

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Fix some tests

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Adjust workaround for type constructors

f85a30d

To solve the warning in initeq.chpl, I needed to change it to use `T(?)`
but that caused a compilation error due to a workaround added in PR chapel-lang#13315

Removing the workaround entirely caused failures for 3 tests:

  classes/ferguson/recursive/issue-12612.chpl
  types/records/recursive/generic-simple.chpl
  types/records/vass/issue-11846-a.chpl

So, the workaround seems necessary in some cases today.

This commit adjusts that workaround to be slightly more focused (now it
requires that the parentSymbol is a TypeSymbol) so that the 3 tests
above can use it but it does not interfere with initeq.chpl.

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>


          Fix .bad file now skipping warning for fields in owned

897cf70

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf force-pushed the warn-generic-fields-vars4 branch from d9f0142 to 897cf70 Compare

July 18, 2023 18:50

lydia-duncan reviewed

View reviewed changes

test/studies/adventOfCode/2021/bradc/futures/day18-bug2.bad Show resolved Hide resolved

mppf mentioned this pull request

Add a warning for generic superclasses that don't include (?) #22784

Merged

1 task

bradcray reviewed

View reviewed changes

test/functions/generic/genericChannelInInit2.good

		@@ -1,3 +1,5 @@
		genericChannelInInit2.chpl:7: warning: please use '?' when declaring a field with generic type
		genericChannelInInit2.chpl:7: note: for example with 'fileWriter(?)'

Member

bradcray Jul 20, 2023

@mppf: Mostly rhetorical question for me, but may be of greater interest to a reviewer: How did you decide whether to keep a test working by updating its source vs. its .good/.bad file?

Member Author

mppf Jul 26, 2023

Generally speaking, if the .good file indicated that the test was for some sort of error message with an erroneous program, then I updated the .good file. If the test seemed to be something that we want to run & continue working, I updated the test itself.

mppf requested a review from vasslitvinov

July 26, 2023 16:39

vasslitvinov approved these changes

View reviewed changes

Member

vasslitvinov left a comment

Looks good. Please see my comment.

compiler/resolution/functionResolution.cpp

+                       t == dtIteratorRecord || t == dtIteratorClass ||
+                       t == dtAnyPOD ||
+                       t == dtOwned || t == dtShared ||
+                       t == dtAnyRecord;

Member

vasslitvinov Jul 26, 2023

Please double-check whether the above needs dtBorrowed, dtUnmanaged, dtAnyManagementAnyNilable, dtAnyNilable(??).

Member Author

mppf Jul 26, 2023

These are covered by isBuiltinGenericClassType which is called as the first option in the || clauses.

mppf merged commit 5779360 into chapel-lang:main

mppf deleted the warn-generic-fields-vars4 branch

July 27, 2023 14:24

mppf added a commit that referenced this pull request


          Add a warning for generic superclasses that don't include (?) (#22784)

ec7d920

This PR takes two steps:

1. It makes it legal to write `class C : GenericParent(?)` in order to
clearly indicate that `class C` is generic due to inheriting from a
generic class.
2. It adds a warning to request this form when inheriting from a generic
class, and a related error for the case of `class C :
ConcreteParent(?)`.

The rationale for the warning is that, if code compiling without these
warnings (including the field warning from PRs #22745 and #22697) then
it is syntactically apparent whether or not a class or record is
generic. That means that, in the future, if we make these errors, a) the
compiler doesn't have to work as hard to know if a type is generic and
b) neither do users.

Reviewed by @vasslitvinov - thanks!

- [x] full comm=none testing

mppf added a commit to mppf/chapel that referenced this pull request


          Add note to new warnings about being an error in the future

Adjusts the warnings from PRs chapel-lang#22697 chapel-lang#22745 and chapel-lang#22784 to include a note
about the warning possibly being an error in the future when compiling
with `--warn-unstable`.

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>

mppf mentioned this pull request

Add note to new warnings about being an error in the future #22877

Merged

1 task

mppf added a commit that referenced this pull request


          Add note to new warnings about being an error in the future (#22877)

de1b56d

Adjusts the warnings from PRs #22697 #22745 and #22784 to include a note
about the warning possibly being an error in the future when compiling
with `--warn-unstable`.

Reviewed by @lydia-duncan - thanks!

- [x] full comm=none testing

mppf mentioned this pull request

Warn for generic declared return types without (?) #23356

Merged

2 tasks

mppf added a commit that referenced this pull request


          Warn for generic declared return types without (?) (#23356)

6eb2f16

For Cray/chapel-private#5229 /
Cray/chapel-private#4736 (comment)

Continuing #22745 and #23291, this PR adds a warning for routines
declared with a generic return type that does not include `(?)`. Note
that we already have a number of problems using `domain` or `domain(?)`
as a return or variable type (#23355, #23357, #23358); as a result
`domain(?)` cannot currently be used as a declared return type.

While there, this PR adds checking to fix #23346 by making `(?)` on a
concrete formal type into a compilation error. It also adds testing for
the variable and return type cases to make sure that they also result in
a compilation error.

For example:

``` chapel
record R { type t; }

proc a() : R { // warning
  return new R(int);
}
a();

proc b() : R(?) { // OK
  return new R(int);
}
b();

proc returnGenericType() type { return R; }

proc c() : returnGenericType() { // warning
  return new R(int);
}
c();

proc d() : returnGenericType()(?) { // OK
  return new R(int);
}
d();
```

Reviewed by @vasslitvinov and @lydia-duncan - thanks!

- [x] full comm=none testing
- [x] full gasnet testing

mppf mentioned this pull request

Update language specification to describe T(?) #23475

Merged

mppf added a commit that referenced this pull request


          Update language specification to describe T(?) (#23475)

6f1b6be

This PR updates the language specification to cover the changes in PRs
#23291 #22784 #22745 #23356 where marking a generic type with `(?)` is
now needed.

This PR adds several new spec sections:
 * Fields with Generic Types
 * Fully Defaulted Generic Types
 * Marking Generic Types

Reviewed by @lydia-duncan - thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet