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.

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

How to / should we preserve old-style runtime nil checking? #13625

Open
vasslitvinov opened this issue Aug 1, 2019 · 1 comment
Open

How to / should we preserve old-style runtime nil checking? #13625

vasslitvinov opened this issue Aug 1, 2019 · 1 comment

Comments

@vasslitvinov
Copy link
Member

The following code, compiled in the --legacy-nilable-classes mode:

var x: MyClass;
... possibly set x to a non-nil ...
x.myMethod();

upon x.myMethod() at run time invokes the nil check that we have had for a long time, one that is disabled upon --fast.

When switching to --no-legacy-nilable-classes, x has to be given a nilable type and the method call has to be preceded by x!. This is a different kind of check.

This issue inquires whether the old-style runtime nil check will still live in the world where there is no --legacy-nilable-classes, and if so, how to write a test to exercise it.

Right now it is tested with:

test/classes/diten/callNilClassesMethod.chpl
test/classes/diten/nilDynamicDispatch.chpl

Meta: this is applicable only when we remove the legacy-nilable-classes option.
@vasslitvinov vasslitvinov mentioned this issue Aug 1, 2019
Closed
37 tasks
@mppf
Copy link
Member

mppf commented Aug 2, 2019

I don't think this question is urgent in any way. The old-style runtime check can be there, even if it is unnecessary, and it will be omitted on --fast compiles so it arguably shouldn't impact performance-oriented compiles. I think we should revisit this after the 1.20 release.

@mppf mppf mentioned this issue Aug 2, 2019
Open
vasslitvinov added a commit to vasslitvinov/chapel that referenced this issue Oct 5, 2019
@vasslitvinov
Fix some tests to work without --legacy-classes
fdc8d72 
For 12 of these tests, I added `?` where the code was using non-nilable types
incorrectly. The remaining fixes are:

* classes/errors/newValueNotType.chpl - I merely adjusted .bad
* deprecated/arrayAsVec/push-back-owned.chpl - no changes, just removed .compopts

Here is my tally of the remaining compopts with `--legacy-classes`,
as of 92fc605 - chapel-lang#13598 needs updating:

* The compile-time control-flow-based nil checking works differently
  in the presence of nilable types:

    classes/delete-free/nil-checking/nil-check-control.chpl
    classes/delete-free/nil-checking/nilcheck-empty-refs.chpl
    classes/delete-free/nil-checking/nilcheck.chpl

* Compile-time control-flow-based nil checking, this is a wish item:

    classes/vass/if-object-2.chpl

* Related to owned/shared - I suspect straightforward to update:

    classes/ferguson/class-double-modifier.chpl

* Old-style runtime nil checking chapel-lang#13625, intentionally legacy:

    classes/diten/callNilClassesMethod.chpl
    classes/diten/nilDynamicDispatch.chpl

* Related to nilability; probably intentionally legacy:

    classes/delete-free/default-arg-typeless-error.chpl
    classes/delete-free/default-arg-typeless-legacy.chpl

* Intentional legacy checking:

    classes/errors/newValueNotType2.chpl
    compflags/ferguson/unstable-class.chpl
    compflags/ferguson/unstable-owned-shared-intent2.chpl
@vasslitvinov vasslitvinov mentioned this issue Oct 5, 2019
Merged
vasslitvinov added a commit that referenced this issue Oct 7, 2019
@vasslitvinov
Merge pull request #14222 from vasslitvinov/remove-some-legacy-compopts
a93ab75 
Fix some tests to work without --legacy-classes

For 12 of these tests, I added `?` where the code was using non-nilable types
incorrectly. The remaining fixes are:

* classes/errors/newValueNotType.chpl - I merely adjusted .bad
* deprecated/arrayAsVec/push-back-owned.chpl - no changes, just removed .compopts

Here is my tally of the remaining compopts with `--legacy-classes`,
as of 92fc605 - #13598 needs updating:

* The compile-time control-flow-based nil checking works differently
  in the presence of nilable types:

      classes/delete-free/nil-checking/nil-check-control.chpl
      classes/delete-free/nil-checking/nilcheck-empty-refs.chpl
      classes/delete-free/nil-checking/nilcheck.chpl

* Compile-time control-flow-based nil checking, this is a wish item:

      classes/vass/if-object-2.chpl

* Related to owned/shared - I suspect straightforward to update:

      classes/ferguson/class-double-modifier.chpl

* Old-style runtime nil checking #13625, intentionally legacy:

      classes/diten/callNilClassesMethod.chpl
      classes/diten/nilDynamicDispatch.chpl

* Related to nilability; probably intentionally legacy:

      classes/delete-free/default-arg-typeless-error.chpl
      classes/delete-free/default-arg-typeless-legacy.chpl

* Intentional legacy checking:

      classes/errors/newValueNotType2.chpl
      compflags/ferguson/unstable-class.chpl
      compflags/ferguson/unstable-owned-shared-intent2.chpl

While there, remove .compopts with `--no-legacy-classes`.

r: @mppf
@vasslitvinov vasslitvinov mentioned this issue Dec 20, 2019
Merged
vasslitvinov added a commit that referenced this issue Dec 23, 2019
@vasslitvinov
Merge pull request #14689 from vasslitvinov/remove--legacy-classes--c…
7a88372 
…ompopts

Remove --legacy-classes from two .compopts

This furthers the goal of eliminating `--legacy-classes` compiler flag.
See also #13625.

This PR modifies these two tests:

    test/classes/diten/callNilClassesMethod.chpl
    test/classes/diten/nilDynamicDispatch.chpl

They used to require --legacy-classes to have a variable of a non-nilable
class type that holds `nil`. Now they use a hole in typechecking of
nilable types to achieve that. Specifically, right now array elements
can be `nil` despite having a non-nilable type.

When this hole is fixed, we can switch to using another hole.
When no holes remain, we can remove altogether the nil-checking being tested
here, and therefore these tests as well.

BACKGROUND

These tests' purpose is to test "old-style" runtime nil checking, which applies
to the receiver upon each method call. To observe it fire, we need
a class variable holding `nil`.

Alas, if this variable has a nilable type, the old-style check will not fire
because:
* the typechecker will insist on calling postfix-`!` on the variable
  before it is passed to a method, and
* postfix-`!` will fire its own error, so the old-style check is never
  executed.

Therefore we need the variable to have a non-nilable type, so that
the typechecker does not insist on calling postfix-`!`.
This change relies on a loophole in the typechecker to make it happen.
Which loophole is used does not matter.

r: @mppf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@mppf @vasslitvinov