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

mppf · 2023-07-13T19:54:34Z

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

--- Signed-off-by: Michael Ferguson <[email protected]>

Necessary for change to DefaultAssociativeDomRehashHelper in upcoming commit. --- Signed-off-by: Michael Ferguson <[email protected]>

--- Signed-off-by: Michael Ferguson <[email protected]>

For test/classes/jade/owned/assign-op.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

--- Signed-off-by: Michael Ferguson <[email protected]>

For test/functions/intents/out/out-generic-type-set-assign.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

For test/types/records/generic/warn-field-generic-declared-type.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

Makes the code clearer and avoids the new warning. --- Signed-off-by: Michael Ferguson <[email protected]>

--- Signed-off-by: Michael Ferguson <[email protected]>

For test/types/partial/bad-managed-init.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

--- Signed-off-by: Michael Ferguson <[email protected]>

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 <[email protected]>

--- Signed-off-by: Michael Ferguson <[email protected]>

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

bradcray · 2023-07-20T22:11:51Z

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(?)'


@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?

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.

vasslitvinov

Looks good. Please see my comment.

vasslitvinov · 2023-07-26T20:24:40Z

compiler/resolution/functionResolution.cpp

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


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

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

@vasslitvinov

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

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 <[email protected]>

@lydia-duncan

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

@vasslitvinov

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

@lydia-duncan

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!

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 <[email protected]>

Improve handling for ? in type exprs

ea3de9c

Necessary for change to DefaultAssociativeDomRehashHelper in upcoming commit. --- Signed-off-by: Michael Ferguson <[email protected]>

Add (?) to two field types in internal modules

bd68e8a

--- Signed-off-by: Michael Ferguson <[email protected]>

Add use of generic type formal to test

0ae82b5

--- Signed-off-by: Michael Ferguson <[email protected]>

Skip warning for integral etc, better detect fields

6f0c2db

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix missing & redundant warnings

4b321b9

--- Signed-off-by: Michael Ferguson <[email protected]>

Update a few tests

c78c1b5

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix generic-field.chpl

d8c18e8

--- Signed-off-by: Michael Ferguson <[email protected]>

Adjust normalizer to compute FLAG_MARKED_GENERIC

58f67f6

For test/classes/jade/owned/assign-op.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

Don't warn for 'var x: record'

f2129dc

--- Signed-off-by: Michael Ferguson <[email protected]>

Don't warn for generic out formals

257e31c

For test/functions/intents/out/out-generic-type-set-assign.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

Avoid redundant field-with-generic-management warnings

f79e984

For test/types/records/generic/warn-field-generic-declared-type.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

Update SortedMap to use a concrete type for _set

cb976c4

Makes the code clearer and avoids the new warning. --- Signed-off-by: Michael Ferguson <[email protected]>

Update tests per new warning

231b564

--- Signed-off-by: Michael Ferguson <[email protected]>

Use concrete type for sendWaiters nd recvWaiters

d82a486

--- Signed-off-by: Michael Ferguson <[email protected]>

Update tests for new warning

faa9c9b

--- Signed-off-by: Michael Ferguson <[email protected]>

Suppress warning for fields of owned/shared

cbc8a71

For test/types/partial/bad-managed-init.chpl --- Signed-off-by: Michael Ferguson <[email protected]>

Fix more tests for the new warning

d3a3fef

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix some tests

5338049

--- Signed-off-by: Michael Ferguson <[email protected]>

Fix .bad file now skipping warning for fields in owned

897cf70

--- Signed-off-by: Michael Ferguson <[email protected]>

mppf force-pushed the warn-generic-fields-vars4 branch from d9f0142 to 897cf70 Compare July 18, 2023 18:50

lydia-duncan reviewed Jul 18, 2023

View reviewed changes

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

mppf mentioned this pull request Jul 20, 2023

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

Merged

1 task

bradcray reviewed Jul 20, 2023

View reviewed changes

mppf requested a review from vasslitvinov July 26, 2023 16:39

vasslitvinov approved these changes Jul 26, 2023

View reviewed changes

mppf merged commit 5779360 into chapel-lang:main Jul 27, 2023
7 checks passed

mppf deleted the warn-generic-fields-vars4 branch July 27, 2023 14:24

mppf mentioned this pull request Aug 4, 2023

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

Merged

1 task

mppf mentioned this pull request Sep 13, 2023

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

Merged

2 tasks

mppf mentioned this pull request Sep 20, 2023

Update language specification to describe T(?) #23475

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

mppf commented Jul 13, 2023 •

edited

Loading

bradcray Jul 20, 2023

mppf Jul 26, 2023

vasslitvinov left a comment

vasslitvinov Jul 26, 2023

mppf Jul 26, 2023

		@@ -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(?)'

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

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

Conversation

mppf commented Jul 13, 2023 • edited Loading

bradcray Jul 20, 2023

Choose a reason for hiding this comment

mppf Jul 26, 2023

Choose a reason for hiding this comment

vasslitvinov left a comment

Choose a reason for hiding this comment

vasslitvinov Jul 26, 2023

Choose a reason for hiding this comment

mppf Jul 26, 2023

Choose a reason for hiding this comment

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

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

mppf commented Jul 13, 2023 •

edited

Loading