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

Type inference: error-causing replacement node doesn't become invalid #35630

Open
askeksa-google opened this Issue Jan 11, 2019 · 2 comments

Comments

Projects
None yet
2 participants
@askeksa-google
Copy link
Contributor

askeksa-google commented Jan 11, 2019

When a node is replaced by a different node during type inference, and the replacement node is then involved in an error condition (such as a type error), the erroneous node is kept in the tree, rather than being replaced by an invalid node, as it should.

For example, consider this program (with set literals enabled):

List<int> top = {};

main() {
  List<int> local = {};
  top = {};
  local = {};
  print(top);
  print(local);
}

The initializer of the local variable gets correctly replaced by an invalid node, but all of the other three assignments keep the set literal.

This issue causes the front-end test strong/inference/infer_from_complex_expressions_if_outer_most_value_is_precise to fail, as it is expecting the invalid node.

@kmillikin

This comment has been minimized.

Copy link
Member

kmillikin commented Jan 11, 2019

This is a basic flaw in the Kernel shadow nodes. They have apparent "children" that are not really children of the shadow node in the sense the the child's parent pointer is the shadow node. Once we begin doing parent-based rewriting of the tree during type inference, we cannot safely use these shadow node "children" because they are stale.

A while ago I added a comment:

        // Do not use rhs after this point because it may be a Shadow node
        // that has been replaced in the tree with its desugaring.

There should be an identical comment just before this line:

      // Do not use rhs after this point because it may be a Shadow node
      // that has been replaced in the tree with its desugaring.
      var replacedRhs = inferrer.ensureAssignable(
          writeContext, rhsType, rhs, writeOffset,
          isVoidAllowed: writeContext is VoidType);
      _storeLetType(inferrer, replacedRhs ?? rhs, rhsType);

where the problem is that there are two occurrences of rhs after that.

@kmillikin

This comment has been minimized.

Copy link
Member

kmillikin commented Jan 11, 2019

(I should add that that comment is misleading. It is not only shadow nodes that are rewritten as part of type inference.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment