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

Improve handling for tryResolve and errors to handle a bad init= #14887

Merged
merged 25 commits into from
Feb 18, 2020

Conversation

mppf
Copy link
Member

@mppf mppf commented Feb 11, 2020

In the event that init= indicates that a type is not copyable
(currently by including a compilerError in the body), the compiler
should raise an error if the copy is used but not if the copies are all
removed. Since some copies are actually removed during callDestructors,
this PR extends the work of PR #8577 with FLAG_ERRONEOUS_INITCOPY.

Changes are:

  • factor some ast-location code into compiler/util/astlocs.cpp from
    baseAST.cpp and misc.cpp
  • add a userInstantiationPointLoc to TypeSymbol and FnSymbol - used
    strictly for error messages - and add getUserInstantiationPoint to
    astlocs.cpp to help with this
  • change from FLAG_ERRONEOUS_AUTOCOPY / FLAG_ERRONEOUS_INITCOPY to
    just FLAG_ERRONEOUS_COPY (it's simpler and it can also apply to
    array's chpl_unref)
  • in initializerRules, allow type queries on this (for partial
    instantiations) so that the range constructor can work correctly when
    called on an instantiation
  • remove defaults from resolveUninsertedCall to reduce confusion
  • adjust resolveUninsertedCall to return the result of tryResolve if
    that is the strategy used (so that tryResolve can return NULL, e.g.)
  • check for an error saved in tryResolveErrors in case it's the 2nd time
    an init= (say) was tryResolved. (Otherwise, the error would only be
    encountered the first time, and then it would be saved in the map, but
    the call would resolve to the function containing the error - leading
    the tryResolve to succeed).
  • add fixInstantiationPointAndTryResolveBody and call it from several
    type-directed functions (like resolveAutoCopyEtc, findAssignFn, etc).
    This detects compilerErrors in the called function and sets the
    instantiation point appropriately.
  • factor most of the code in preFold for PRIM_IS_COPYABLE etc into
    separate functions
  • set instantiationPoint on the TypeSymbol for a constructed tuple type
  • make handleError accept an astlocT and use that instead of the hacky
    createASTforLineNumber - and while there get one more handleError
    variant to call vhandleError instead of repeating the same code
  • add a compilerError for initCopy for arrays to check that the type is
    assignable
  • adjust range.init to work with (partial) instantiations (so that
    isDefaultInitializable will work correctly on it)
  • add tests of missing init= and arrays of types missing =
  • update several chameneous tests to use nilable types to work around
    issue cannot return array of non-nilable owned #14896

Reviewed by @benharsh - thanks!

  • full local futures testing

modules/internal/ChapelArray.chpl Show resolved Hide resolved
mppf added 23 commits February 18, 2020 06:47
The change to forwarding is based on an internal error I noticed
The trouble is that too many places resolving the function
need to adjust the instantiation point first.
See e.g. test/library/standard/Map/testIterators.chpl which
failed before this change, because the '=' resolved in the array
initializer was not getting the correct instantiation point for the type.
and report initCopy errors from iterators immediately
@mppf mppf merged commit 5389fc8 into chapel-lang:master Feb 18, 2020
@mppf mppf deleted the missing-init-eq branch February 18, 2020 13:53
@mppf
Copy link
Member Author

mppf commented Feb 18, 2020

Just noting that this PR is connected to issue #8065 which discusses how users should indicate a type is not copyable.

mppf added a commit to mppf/chapel that referenced this pull request Feb 19, 2020
This change is causing problems in const-checking of shadow variables
that I'm not quite sure how to resolve at the moment.
mppf added a commit that referenced this pull request Feb 19, 2020
Un-do array init copy change

Changes in array initCopy from PR #14887 caused failures for the test
test/parallel/forall/reduce-intents/ri-a2-AoA.chpl in performance runs.
The problem is related to const-checking of shadow variables that I'm not
quite sure how to resolve at the moment.

This PR removes the intent changes for array initCopy.

Issue #14923 tracks the future work of fixing it (along with the new
future test/types/records/const-checking/array-of-owned-const.chpl)

While there, run the test that failed in performance testing in regular
testing too. Since full testing passed with the PR before it was merged,
this test is testing something different from others.

Trivial and not reviewed.

- [x] full local testing
e-kayrakli added a commit that referenced this pull request Feb 21, 2020
Add performance annotations

Adds annotation for the following perf changes and the responsible PRs

- Gasnet Performance improvements on cs xc
    #14912
- Memory leak
    #14907
- Compiler performance
    #14887
- String temporary copy performance
    #14903 
- Unordered GET improvements on ofi
    #14810

[Reviewed by @ronawho]
mppf added a commit that referenced this pull request Mar 19, 2020
Fix up chameneous benchmarks and patterns of returning array of non-nilable

Related: PR #14887, issue #14896, PR #15240.

This PR reverts some changes to chameneos tests now that issue #14896 is 
solved. Once the changes were reverted, the tests uncovered additional
problems with the erroneous-initializer logic that this PR fixes.

Reviewed by @benharsh - thanks!

- [x] full local testing
mppf added a commit that referenced this pull request Mar 19, 2020
Add where false variants of missing = init= tests

For #8065 and following PR #14887.

This PR adds additional tests that indicate a type is not copy-able
with a `where false` on the `init=`.

Test changes only, not reviewed.
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.

2 participants