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

dyno: resolve nested init calls #24906

Merged
merged 14 commits into from
Apr 24, 2024

Conversation

DanilaFe
Copy link
Contributor

@DanilaFe DanilaFe commented Apr 22, 2024

Closes #24854. Closes https://github.com/Cray/chapel-private/issues/6182 (sprint task).

This PR adjusts the InitResolver to support calls like this.init inside other init procedures.

To make this work, the PR does the following things:

  1. It disallows calls to 'init' on anything other than 'super' and 'this'. As far as I know, this is already effectively the standard for Chapel; however, it was not enforced. As a result, the following program -- which is legal on main -- is now illegal.
    record myRec {
        var x: int;
    
        proc init(x: int, y: int) {
            this.x = x + y;
        }
    
        proc init(x: int) {
            init(x, 0);
        }
    
        proc init() {}
    }
    
    record container {
      var r: myRec;
    
      proc init() {
        init this;
        r.init(10); // bad bad bad
      }
    }
    
    var c: container;
    By doing so, the PR makes it possible for the InitResolver to syntactically detect calls such as this.init(). By doing this, in turn, we can start representing their effects on the resolution state.
  2. Adding new handling methods to InitResolver that operate on CallResolutionResults in addition to AST nodes. This is required because the effects of calling this.init depend on which overload of init is called; as such, we want to go through the usual mechanism for resolving the initializer. Doing so using CallResolutionResults makes the most sense. As a result of this PR, the handleCallExpr follows up resolution with handleResoledCall, which descends into the InitResolver.
    • When handling init, the InitResolver populates the individual field states as if they were assigned to. This is useful because the calls to 'this.init' need to fit into the existing framework of initialization: handling branches, merging intermediate results, etc.
    • This also requires a slight adjustment to the handleCallExpr to stop skipping call resolution when the receiver formal is generic. The this in this.init for a generic method is going to be generic, but that should not stop us from performing resolution.

As a result, #24854 is resolved; a major consequence of that is that ranges with the by operator (mentioned in #24853) now resolve correctly: the constructor that previously threw away the stride now properly takes it into account. I was able to validate that the implementation of ranges in Dyno works with strides as one would expect.

Reviewed by @benharsh -- thanks!

Testing

  • paratest
  • dyno tests

Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Also emit errors for fields initialized before 'this.init()' call.

Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
This should make it easier to test non-one stride kinds

Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
Signed-off-by: Danila Fedorin <daniel.fedorin@hpe.com>
@benharsh benharsh self-requested a review April 23, 2024 22:34
@DanilaFe DanilaFe merged commit 5822073 into chapel-lang:main Apr 24, 2024
7 checks passed
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.

dyno: calls to initializers from other initializers aren't reflected.
2 participants