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

Warn for fields declared with generic management or with generic domain type #22697

Merged
merged 14 commits into from
Jul 10, 2023

Conversation

mppf
Copy link
Member

@mppf mppf commented Jul 7, 2023

This PR adds a warning for two cases:

  • fields declared like var x: domain. We want users to write var x: domain(?) to emphasize that domain is a generic type and therefore x is a generic field.
  • fields declared like var x: C, where C is a class name. These have generic management and cause the field to be generic but that is often unexpected and leads to hard-to-understand errors. We want users to specify memory management explicitly to avoid this kind of genericity. This PR does not introduce a warning for the case where memory management is specified, however the class is generic (that is future work).

It adds these warnings in checkSurprisingGenericDecls which I plan to extend in the future for warnings for variable declarations like var x: someGenericRecord;.

For https://github.com/Cray/chapel-private/issues/5031

Some of this PR is based upon earlier work in PR #21404.

Reviewed by @vasslitvinov - thanks!

  • full comm=none testing

mppf added 14 commits July 7, 2023 14:07
avoid copying the typeExpr if it's not going to be used

---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
---
Signed-off-by: Michael Ferguson <mppf@users.noreply.github.com>
@mppf mppf requested a review from vasslitvinov July 7, 2023 20:35
@mppf mppf changed the title Warn for fields declared with generic management / domain Warn for fields declared with generic management or with generic domain type Jul 10, 2023
@mppf mppf merged commit 855d85e into chapel-lang:main Jul 10, 2023
@mppf mppf deleted the warn-fields-generic-mgmt branch July 10, 2023 19:07
@bradcray
Copy link
Member

Unless I missed a PR, doesn't this:

fields declared like var x: domain. We want users to write var x: domain(?) to emphasize that domain is a generic type and therefore x is a generic field.

make domain fields inconsistent with other generic fields, as in this example:

record R { 
  type t;
  var x: t;
}

record S {
  var r: R;  // no warning about R being generic, needing to be spelled `R(?)`
  var s: range;  // maybe OK since range is fully-defaulted?
}

var myS = new S(new R(int), 1..10);
writeln(myS);

Put another way, I thought we would want to start requiring domain(?) in all contexts where we might start requiring (?) to indicate a known-to-be-incomplete type. But with this PR, I think it's become a singleton case where we're requiring it—is that right? (that said, if/when other in-progress PRs are merged, it would cease to be special in this regard?)

@mppf
Copy link
Member Author

mppf commented Jul 17, 2023

@bradcray - yes, that's accurate in terms of var r: R vs var x: domain being inconsistent. I am expecting that we will end up with this warning at least for fields (in both cases). It is easy enough to adjust if we decide something different about domain(?), though.

mppf added a commit that referenced this pull request Jul 27, 2023
)

Resolves Cray/chapel-private#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!

- [x] full comm=none testing

Future work
* warn for `class C : SomeGenericType` -- PR #22784
mppf added a commit that referenced this pull request Jul 27, 2023
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 Aug 4, 2023
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 added a commit that referenced this pull request Aug 8, 2023
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
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.

3 participants