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

[NNBD] Strange CFE error for user-defined ~ operator #42379

Closed
sgrekhov opened this issue Jun 17, 2020 · 4 comments
Closed

[NNBD] Strange CFE error for user-defined ~ operator #42379

sgrekhov opened this issue Jun 17, 2020 · 4 comments
Assignees
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. needs-info We need additional information from the issue author (auto-closed after 14 days if no response) NNBD Issues related to NNBD Release type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)

Comments

@sgrekhov
Copy link
Contributor

The following code produces a warning in analyzer and warning and error in VM

class C {
  C operator ~() {
    return this;
  }

  C e() {
    return this;
  }
}

main() {
  C c = new C();
  ~c?.e();
}

Analyzer output is

Analyzing precedence_16_unary_postfix_t02.dart...
warning - The target expression can't be null, so the null-aware operator '?.' can't be used. - precedence_16_unary_postfix_t02.dart:52:5 - invalid_null_aware_operator
1 warning found.

Dart VM produces

precedence_16_unary_postfix_t02.dart:52:4: Warning: Operand of null-aware operation '?.' has type 'C' which excludes null.
- 'C' is from 'precedence_16_unary_postfix_t02.dart'.
~c?.e();
  ^
precedence_16_unary_postfix_t02.dart:52:3: Error: Operator '~' cannot be called on 'C?' because it is potentially null.
- 'C' is from 'precedence_16_unary_postfix_t02.dart'. 
~c?.e();
 ^

Dart VM version: 2.9.0-16.0.dev (dev) (Tue Jun 16 10:35:10 2020 +0200) on "windows_x64"

@eernstg eernstg added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. type-bug Incorrect behavior (everything from a crash to more subtle misbehavior) labels Jun 17, 2020
@eernstg
Copy link
Member

eernstg commented Jun 17, 2020

It looks like the analyzer gets the syntactic structure wrong. Here's a variant of the example:

class C {
  C operator ~() {
    return this;
  }

  C e() {
    return this;
  }
}

main() {
  C c = new C();
  ~(c?.e());
}

With this version, nullsafety.dartpad.dev reports 'An expression whose value can be 'null' must be null-checked before it can be dereferenced' from the analyzer on line 13, and the front end still reports 'Operator '~' cannot be called on 'C?' because it is potentially null'.

So when the parentheses are added, we get the expected error from both the analyzer and the CFE, but without the parentheses, the message from the analyzer only fits the parsing structure (~c)?.e().

In order for ~c?.e() to have the latter structure, ~c must be a <primary>, which is not the case (it is a <unaryExpression>). But c is a <primary>, and c?.e() is a <unaryExpression>, so ~e?.e() fits <prefixOperator> <unaryExpression>, which is a correct <unaryExpression>.

@scheglov scheglov added the NNBD Issues related to NNBD Release label Jul 9, 2020
@scheglov
Copy link
Contributor

scheglov commented Jul 9, 2020

The language spec says in 16.31 Unary Expressions: Any other expression of the form op e is equivalent to the method invocation e.op(). So, ~c?.e() is equivalent c?.e().~(), and is handled with null-shorting, so it is not a compile-time error.

The NNBD spec says that (e) translates to TERM[(EXP(e))]. So, when ~(c?.e()), the operator ~ is invoked on C?, and this is a compile-time error.

I don't understand the statement about <primary>, etc. The AST structure is PrefixExpression(MethodInvocation(c, e, [], nullAware), ~). Does it look right to you?

@scheglov scheglov self-assigned this Jul 9, 2020
@scheglov scheglov added the needs-info We need additional information from the issue author (auto-closed after 14 days if no response) label Jul 9, 2020
@eernstg
Copy link
Member

eernstg commented Jul 9, 2020

@scheglov wrote:

The language spec says ...

Indeed, it does! However, the language specification uses the same approach for a large number of binary operations, which would make constructs like c?.m() + 1 work in a similar way (such that it is not an error with null-safety, and + will be skipped at run-time if c does evaluate to null). We discussed all those possible extensions of null-shorting and decided that they should not be supported. (One reason was that there is no syntactic support for null-shorting + explicitly, another one was that it gets harder and harder to see where null-shorting ends if it is very inclusive).

So I find it highly unlikely that we will include the binary operations in null-shorting (and the tools actually don't do that today). So we may also decide to exclude these prefix operators, for consistency.

I created dart-lang/language#1081 in order to clarify this question.

@scheglov
Copy link
Contributor

dart-bot pushed a commit that referenced this issue Aug 25, 2020
…ix expressions.

Bug: #42379
Change-Id: I366702f144a4350bed6b83c5c0936359aeeae2c1
Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/159960
Reviewed-by: Brian Wilkerson <brianwilkerson@google.com>
Commit-Queue: Konstantin Shcheglov <scheglov@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. needs-info We need additional information from the issue author (auto-closed after 14 days if no response) NNBD Issues related to NNBD Release type-bug Incorrect behavior (everything from a crash to more subtle misbehavior)
Projects
None yet
Development

No branches or pull requests

3 participants