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

Add isCopyable etc #14842

Merged
merged 10 commits into from
Feb 5, 2020
Merged

Add isCopyable etc #14842

merged 10 commits into from
Feb 5, 2020

Conversation

mppf
Copy link
Member

@mppf mppf commented Jan 31, 2020

Motivated by work in PR #14835, I realized that list needs to avoid
having an init= or = function when the element type is a non-nilable
owned, e.g. owned C. The problem is that in that event, the source list
elements would be transferred out of and then store nil. Additionally
list should not include an init= function if the elements do not
include one.

In order to solve that problem, list will need to be able to query
whether or not the elements are copyable. In addition it makes sense to
be able to query if a type is only copyable from a ref, which is the
case for owned C? (say).

This PR adds the following queries to the Types standard module:

  • isConstCopyable returns true if the type can be initialized from a
    const value (var x = myConstValue)
  • isCopyable returns true if the type can be copy-initialized at all
    (but it might only be initializable from a mutable other variable, as
    is the case with owned)
  • isConstAssignable returns true if the type can be assigned from a
    const value (x = myConstValue)
  • isAssignable returns true if the type can be assigned at all (but it
    might only be assignable from a mutable rhs variable, as is the case
    with owned).
  • isDefaultInitializable returns true if the type has a default value
    (e.g. supports var z: MyType;)

isCopyable and isConstCopyable are chosen because some types
(including map after PR #14793) use the ref-if-modified intent in
init=. For these types, it is not known at resolution time whether the
intent will be ref or const ref.

This PR adds the new functions to Types.chpl and the related primitives
and support in preFold. Flags are computed and applied to a type so that
multiple calls to isCopyable will not result in trying to resolve
init= each time. Additionally, this PR adjusts the expiring value
analysis to avoid visiting task functions twice and tidies some
whitespace in preFold.cpp.

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

  • full local testing

Future Work:

Copy link
Member

@vasslitvinov vasslitvinov left a comment

Choose a reason for hiding this comment

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

The implementation looks good.

Note that your PR message would lead to closing #13195 upon merge. I suggest fixing that unless that's the intention.

@vasslitvinov
Copy link
Member

The interface looks good to me, given that we need to have this information.

The only thing that gave me pause is hasDefaultValue, whereas I wanted to have isDefaultInitializable. In retrospect, hasDefaultValue is pretty good.

I suggest to coordinate the choice of the name for the above with the error that the compiler issues when default-initializing a non-nilable variable or a record without init(). Possibly by adjusting the error. Michael reports that currently the former reads Cannot default-initialize x: owned MyClass.

@dlongnecke-cray
Copy link
Contributor

These routine names look good to me. It looks like hasDefaultValue would provide me with a more general solution for #14367?

@lydia-duncan
Copy link
Member

The only thing that gave me pause is hasDefaultValue, whereas I wanted to have isDefaultInitializable. In retrospect, hasDefaultValue is pretty good.

I think either is fine, but prefer hasDefaultValue for succinctness.

@mppf
Copy link
Member Author

mppf commented Feb 3, 2020

I think if we were to use isDefaultInitializable we would perhaps want e.g. isCopyInitializable.

@mppf
Copy link
Member Author

mppf commented Feb 4, 2020

I have some concern that hasDefaultValue will be confusing when applied to a value.
For example, someone might misunderstand and expect hasDefaultValue(1) to return false because the default integer is 0. I do not thing that isCopyable has this problem.

Therefore I'd like to use isDefaultInitializable and leave the rest as-is.

@lydia-duncan
Copy link
Member

I assumed hasDefaultValue would only take a type argument

@mppf
Copy link
Member Author

mppf commented Feb 4, 2020

I assumed hasDefaultValue would only take a type argument

All of these functions in Types, like isNilableClass or isInt take either a type or a value. I'd like to keep these new functions that way as well.

@dlongnecke-cray
Copy link
Contributor

dlongnecke-cray commented Feb 4, 2020

I think if we were to use isDefaultInitializable we would perhaps want e.g. isCopyInitializable.

While it is a bit more wordy I think that isDefaultInitializable captures the intended meaning a bit better. My gut reaction was the same as Vass's in that regard.

Copy link
Member

@lydia-duncan lydia-duncan left a comment

Choose a reason for hiding this comment

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

Couple of comments related to the future, but otherwise I think this is good

mppf added a commit that referenced this pull request Feb 5, 2020
Adjust tryResolve to return false when compilerError encountered

Resolves #13195.

This PR updates resolution handling of tryResolve to hide compilerErrors
encountered along the way and instead just make tryResolve return false
in that case. Additionally, it adjusts tryResolve to include a boolean
argument indicating if the body of the called function should also be
resolved (so that if it contains a compilerError call we can handle that
appropriately). The PR also makes some adjustments to misc.cpp towards
making it possible to capture and re-issue all errors but that is not
currently used. However I am including it here because I think it is an
improvement to the structure of error reporting.

This PR takes no particular action for resolving functions called by the
function being attempted. These are resolved in whatever order resolution
gets to them normally.

I started on this patch because in working on #13600 I needed a way to
mark an `init=` or `=` function as not resolveable (see #8065) so that
isCopyable etc (from PR #14842) can return false.

Future work:

Answer the open design questions in this area
 1. Should canResolve always try to resolve the body of the called
    function?
 2. Should canResolve always try to resolve the body of the called
    function and all functions called within it, recursively?
 3. Should canResolve return false for other errors besides
    compilerError?

- [x] full local testing

Reviewed by @lydia-duncan - thanks!
@mppf mppf merged commit 914cf8f into chapel-lang:master Feb 5, 2020
@mppf mppf deleted the isCopyable branch February 5, 2020 20:39
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.

4 participants