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

Unsound type check: it compiles but fails at runtime #51680

Closed
iazel opened this issue Mar 9, 2023 · 13 comments
Closed

Unsound type check: it compiles but fails at runtime #51680

iazel opened this issue Mar 9, 2023 · 13 comments

Comments

@iazel
Copy link

iazel commented Mar 9, 2023

Dart SDK version: 2.19.2 (stable) (Tue Feb 7 18:37:17 2023 +0000) on "linux_x64"


UPDATE: implementing it as an extension function works as expected. Quite odd.


Hello,

I've implemented the usual Result<T, E> type. In case you are unfamiliar with it, it is just a type that carries the result of a computation that could fail. You can think of it as an exception moved into data.

Anyway, I've defined it as such:

enum ResultType { ok, error }

class Result<T, E> {
  final ResultType type;

  const Result(this.type);
 
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T data) f) {
    switch (type) {
      case ResultType.ok:
        return f((this as ResultOk<T>).value);
      case ResultType.error:
        return this as Result<T2, E>;
    }
  }
}

class ResultOk<T> extends Result<T, Never> {
  final T value;

  ResultOk(this.value) : super(ResultType.ok);
}

class ResultErr<E> extends Result<Never, E> {
  final E error;

  const ResultErr(this.error) : super(ResultType.error);
}

We can then write a quick test for it:

test('happy case', () {
  final Result<int, String> ok = ResultOk(10);
  final Result<String, String> res = ok.andThen((n) {
    if (n % 2 == 0) {
      return ResultOk(n.toString());
    } else {
      return const ResultErr("not even");
    }
  });

  expect(res, isA<ResultOk>());
});

Everything will compile, but once we run it, it fails with this error message:

type '(int) => Result<String, String>' is not a subtype of type '(int) => Result<String, Never>' of 'f'

The simpler map function, and similar, works as expected:

Result<T2, E> map<T2>(T2 Function(T) f) {
  switch (type) {
    case ResultType.ok:
      return ResultOk(f(this as ResultOk<T>).value));
    case ResultType.error:
      return this as Result<T2, E>;
  }
}

Interestingly enough, defining andThen as a function works as expected:

Result<T2, E> andThen<T, T2, E>(
    Result<T, E> r,
    Result<T2, E> Function(T) f,
) {
  switch (r.type) {
    case ResultType.ok:
      return f((r as ResultOk<T>).value);
    case ResultType.error:
      return r as Result<T2, E>;
  }
}

test('happy case', () {
  final Result<int, String> ok = ResultOk(10);
  final Result<String, String> res = andThen(ok, (int n) {
    if (n % 2 == 0) {
      return ResultOk(n.toString());
    } else {
      return const ResultErr("not even");
    }
  });

  expect(res, isA<ResultOk>());
});

Notice that this time n in not properly inferred, hence we have to manually specify it, but at least this is caught by both analyzer and compiler.

Hope the issue is clear,
Thanks for your work and support!

@ds84182
Copy link
Contributor

ds84182 commented Mar 9, 2023

Implementing it as an extension method works because of the difference between static type arguments and runtime type arguments.

Inside of Result, T and E reflect the type argument at construction, not declaration.

@eernstg
Copy link
Member

eernstg commented Mar 9, 2023

The basic issue here is dynamically checked covariance. The following version of the basic declarations type checks with --enable-experiment=variance, and it shows where there is a need for invariance rather than covariance (in order to avoid the type checks at run time), namely at inout:

// `--enable-experiment=variance`.

enum ResultType { ok, error }

class Result<out T, inout E> {
  final ResultType type;

  const Result(this.type);

  Result<T2, E> andThen<T2>(Result<T2, E> Function(T data) f) {
    switch (type) {
      case ResultType.ok:
        return f((this as ResultOk<T>).value);
      case ResultType.error:
        return this as Result<T2, E>;
    }
  }
}

class ResultOk<out T> extends Result<T, Never> {
  final T value;

  ResultOk(this.value) : super(ResultType.ok);
}

class ResultErr<inout E> extends Result<Never, E> {
  final E error;

  const ResultErr(this.error) : super(ResultType.error);
}

The reason why E must be invariant is that it occurs in a covariant position in the parameter type of f in andThen (as long as E is considered covariant), which gives rise to a dynamic check on the actual argument passed to f.

In the invocation that fails at run time, the function literal (n) {...} which is passed to andThen gets the inferred parameter type int and return type Result<String, String>, but the actual parameter type of ok.andThen is Result<String, Never> Function(int). So the actual value of f doesn't have the type which is required for f, and hence you get a dynamic error.

You can use the variance feature to look into the details about where you are using dynamically checked covariance in the example. The first thing is that final Result<int, String> ok = ResultOk(10); is a compile-time error because ResultOk(10) has type Result<int, Never>, and that's no longer a subtype of Result<int, String> when we have enabled statically checked variance and made E invariant.

PS: Vote for dart-lang/language#524 in order to help adding support for static checking in this kind of situation. ;-)

@iazel
Copy link
Author

iazel commented Mar 9, 2023

Thanks to the both of you for the explanations.

I hope the variance proposal will go through soon, it's awful when the compiler doesn't tell you it will for sure crash your program at some point.

@eernstg
Copy link
Member

eernstg commented Mar 9, 2023

Thanks!

By the way, I think the Result hierarchy can be expressed in a more standard OO manner. I changed the type arguments because it won't work to insist that a ResultOk must forget the error type. On the other hand it's OK for a ResultErr to forget the OK type, because there is no way (with the given operations) we can come back to the "OK track" when an error has occurred.

abstract class Result<out T, inout E> {
  const Result();
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f);
  Result<T2, E> map<T2>(T2 Function(T) f);
}

class ResultOk<out T, inout E> implements Result<T, E> {
  final T value;
  ResultOk(this.value);
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f) => f(value);
  Result<T2, E> map<T2>(T2 Function(T) f) => ResultOk(f(value));
}

class ResultErr<inout E> extends Result<Never, E> {
  final E error;
  const ResultErr(this.error);
  Result<T2, E> andThen<T2>(Result<T2, E> Function(Never) f) => this;
  Result<T2, E> map<T2>(T2 Function(Never) f) => this;
}

void main() {
  final ok = ResultOk<int, String>(10);
  final res = ok.andThen((n) {
    if (n.isEven) return ResultOk(n.toString());
    return ResultErr("not even");
  });
  print('Result type: ${res.runtimeType}');
}

The out and inout keywords can be deleted if there is no support for statically checked variance, but the fact that they are there now helps checking that the design as a whole doesn't have variance issues.

@iazel
Copy link
Author

iazel commented Mar 10, 2023

@eernstg Thanks for your suggestion, I did try it already, but couldn't find a good solution for another operation I need:

Result<T, E2> mapErr<E2>(
    Result<T, E2> Function(E) f
);

I couldn't find a way for ResultOk to work it out 🤔

@eernstg
Copy link
Member

eernstg commented Mar 10, 2023

I think you'll have to preserve both type arguments in order to allow for mapErr, because that method has a contravariant occurrence of the OK type (as well as a covariant one). This is not surprising, because this means that the argument f to mapErr is allowed to "switch back to the OK track, so it needs to know T".

abstract class Result<inout T, inout E> {
  const Result();
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f);
  Result<T2, E> map<T2>(T2 Function(T) f);
  Result<T, E2> mapErr<E2>(Result<T, E2> Function(E) f);
}

class ResultOk<inout T, inout E> implements Result<T, E> {
  final T value;
  ResultOk(this.value);
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f) => f(value);
  Result<T2, E> map<T2>(T2 Function(T) f) => ResultOk(f(value));
  Result<T, E2> mapErr<E2>(Result<T, E2> Function(E) f) => ResultOk(value);
}

class ResultErr<inout T, inout E> extends Result<T, E> {
  final E error;
  const ResultErr(this.error);
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f) => ResultErr(error);
  Result<T2, E> map<T2>(T2 Function(T) f) => ResultErr(error);
  Result<T, E2> mapErr<E2>(Result<T, E2> Function(E) f) => f(error);
}

void main() {
  final ok = ResultOk<int, String>(10);
  final res = ok.andThen<String>((n) {
    if (n.isEven) return ResultOk(n.toString());
    return ResultErr("not even");
  });
  print('Result type: ${res.runtimeType}');
}

@iazel
Copy link
Author

iazel commented Mar 10, 2023

I was so focused on returning this that I forgot I could just create a new ResultOk 🤦

Thank you!

@iazel
Copy link
Author

iazel commented Mar 10, 2023

@eernstg sorry to further bother you, but I am having trouble enabling the experimental flag on Android Studio. Do you, by any chance, have any knowledge of it?

I did set it in analysis_options.yml file and errors are coming out, but it doesn't recognize inout keywords, it says:

error: This requires the experimental 'variance' language feature to be enabled. (experiment_not_enabled_off_by_default

This is what I have on the analysis file:

analyzer:
  exclude: [build/**]
  language:
    strict-casts: true
    strict-inference: true
    strict-raw-types: true
  enable-experiment:
    - variance

@eernstg
Copy link
Member

eernstg commented Mar 10, 2023

I don't think it is possible to enable experiments using analysis_options.yaml, it's given on the command line, using --enable-experiment=<comma_separated_list_of_experiment_names>, and it must be done on a version of the SDK that has the experiment (they are disabled on stable releases, IIRC).

@eernstg
Copy link
Member

eernstg commented Mar 10, 2023

By the way, you can always create a good approximation of invariance (if you're willing to pay a little bit extra at run time, and willing to dive into less-than-obvious error messages wherever the invariance is violated):

abstract class _Result<T, E, Invariance extends _Inv<T, E>> {
  const _Result();
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f);
  Result<T2, E> map<T2>(T2 Function(T) f);
  Result<T, E2> mapErr<E2>(Result<T, E2> Function(E) f);
}

class _ResultOk<T, E, Invariance extends _Inv<T, E>> implements
    _Result<T, E, Invariance> {
  final T value;
  _ResultOk(this.value);
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f) => f(value);
  Result<T2, E> map<T2>(T2 Function(T) f) => ResultOk(f(value));
  Result<T, E2> mapErr<E2>(Result<T, E2> Function(E) f) => ResultOk(value);
}

class _ResultErr<T, E, Invariance extends _Inv<T, E>> implements
    _Result<T, E, Invariance> {
  final E error;
  const _ResultErr(this.error);
  Result<T2, E> andThen<T2>(Result<T2, E> Function(T) f) => ResultErr(error);
  Result<T2, E> map<T2>(T2 Function(T) f) => ResultErr(error);
  Result<T, E2> mapErr<E2>(Result<T, E2> Function(E) f) => f(error);
}

typedef _Inv<T1, T2> = (T1, T2) Function(T1, T2);

typedef Result<T, E> = _Result<T, E, _Inv<T, E>>;
typedef ResultOk<T, E> = _ResultOk<T, E, _Inv<T, E>>;
typedef ResultErr<T, E> = _ResultErr<T, E, _Inv<T, E>>;

@eernstg
Copy link
Member

eernstg commented Mar 14, 2023

I think I'll close this issue: The dynamically checked covariance is known, and this issue plus one possible fix is covered by dart-lang/language#524.

@eernstg eernstg closed this as completed Mar 14, 2023
@iazel
Copy link
Author

iazel commented Mar 14, 2023

Ok, I'm fine with that, however is the linked issue the one you intended? I can't see the connection between this and 524

@eernstg
Copy link
Member

eernstg commented Mar 14, 2023

Fixed the link!

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

No branches or pull requests

3 participants