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

Updates towards enabling nilability checks #13610

Merged
merged 16 commits into from
Aug 3, 2019

Conversation

mppf
Copy link
Member

@mppf mppf commented Aug 1, 2019

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

This PR addresses about the first 100 failures with --no-legacy-nilable-clases. We are working on getting clean testing in that mode in preparation for enabling it by default.

Compiler changes:

  • normalizes e.g. _owned(anymanaged MyClass) to _owned(borrowed MyClass)
  • improves error checking for PRIM_INIT_VAR and for errorIfNonNilableType when checking for default initialization. In particular, allow some default-init/assign patterns through the use of the flag FLAG_INITIALIZED_LATER.

Module changes:

  • use nilable types in SparseBlockDom
  • bulkAdd_help uses a locale? in the addOn argument
  • Add error for setting a class type to nil via cast or =
  • Adjusts makeArrayFromOpaque for nilibility
  • Adds a work-around for associative arrays of owned
  • Adjusts DistributedBag / DistributedDeque for nilability
  • Adjusts the TOML module for nilability and to remove = overloads on classes (see Overloading assignment (=) on class types #5358 (comment) ) and adjusts mason code to use the new TOML set methods
  • Adjusts ZMQ.chpl to compile with nilability checking enabled by adjusting the initializers to remove the possibility that any fields store nil.
  • Adjusts the conditional in the read function for a channel to make a copy of the argument to read in to - in order avoid default initializing which does not work for non-nilable types.
  • Adjusts the Spawn module to always initialize the home field

Additionally adjusts a number of tests to pass with --no-legacy-nilable-classes.

  • full local testing
  • futures testing

mppf added 16 commits August 1, 2019 15:27
See
  classes/deitz/class-numbered/class1-old.chpl
  classes/marybeth/test_dispatch1-error.chpl

which were both reporting the wrong errors.
* Avoids doing retain/release since these relied on 'nil'
* Added makeClass to one initializer to work around initializers
  being confused by on statements.
See test/expressions/if-expr/inside-field.chpl
Works around an issue in
 library/standard/DataFrames/psahabu/HelloDataFrame

related to associative arrays of owned.
See param/bharshbarg/paramFieldType
@mppf
Copy link
Member Author

mppf commented Aug 1, 2019

@ben-albrecht - would you please review the TOML/Mason changes?

@lydia-duncan - could you review the makeArrayFromOpaque and ZMQ changes?

@LouisJenkinsCS - FYI about DistributedBag and DistributedDeque changes. Here I took the most obvious approach of using nilable class types MyClass? and then adding ! when calling methods etc. It's likely this could be improved upon in a separate effort (e.g. fields can be non-nilable if they are always initialized).

@benharsh - could you look at the compiler changes and anything else?

Thanks!

@LouisJenkinsCS
Copy link
Member

Does the ! stuff add any overhead?

@LouisJenkinsCS
Copy link
Member

LouisJenkinsCS commented Aug 1, 2019

Honestly if it forces a nil check, is there a way to opt-out of these checks? The bag is meant to be a high-performance data structure, no nil checks are needed, I'd wager.

Also @mppf can you fulfill #13257

@mppf
Copy link
Member Author

mppf commented Aug 1, 2019

Does the ! stuff add any overhead?

These run a nil check that is disabled under --fast.

Copy link
Member

@ben-albrecht ben-albrecht left a comment

Choose a reason for hiding this comment

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

TOML/mason changes looked reasonable to me.

@lydia-duncan
Copy link
Member

could you review the makeArrayFromOpaque and ZMQ changes?

The makeArrayFromOpaque changes look right. I think the ZMQ changes are good, too, but I'm a little less confident in those since I don't think our testing of it is particularly robust. If it's not too much trouble, would you be up for adding a test about a way it might go wrong based on your changes?

@mppf mppf requested a review from benharsh August 2, 2019 16:46
@mppf mppf mentioned this pull request Aug 2, 2019
1 task
@vasslitvinov vasslitvinov merged commit db3049a into chapel-lang:master Aug 3, 2019
@mppf mppf deleted the fix-two-errors branch August 8, 2019 16:05
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.

6 participants