Dart 2.0 Support

[devkabiir]

Hi @polux, All test are passing.
I know these are a lot of changes to review, so I suggest going commit by commit. I've tried to keep them smaller and stick to what the message says.
I've raised the sdk constraints to >=2.0.0 Not sure if anyone needs it to be lower than that. Also didn't have time to test it against Dart 1. But it should work! I removed new and const operators, so it won't
I fixed 1 example too full_arith.

Closes #23

[polux]

Thanks for this great contribution! I'm wondering: which version of dart are you using? With v2.3.0.dev.0.1 I'm getting an error when running the full_arith example (which I understand you have fixed):

Unhandled exception:
type '() => dynamic' is not a subtype of type '() => Parser<dynamic>'
#0      Arith.expr (file:///tmp/parsers/example/full_arith.dart:29:17)
#1      Arith.start (file:///tmp/parsers/example/full_arith.dart:20:16)
#2      main (file:///tmp/parsers/example/full_arith.dart:42:17)
#3      _startIsolate.<anonymous closure> (dart:isolate-patch/isolate_patch.dart:300:19)
#4      _RawReceivePortImpl._handleMessage (dart:isolate-patch/isolate_patch.dart:171:12)

[devkabiir]

@polux I'm using Dart stable 2.2.0

[devkabiir]

Looks like 2.3+ adds more type checks. 🤦‍♂️ Should've tested against it. Will probably release soon.

[polux]

The good news is the tests pass on 2.3. Would you mind fixing the full_arith example before I merge?

Also I'm getting errors when running test/run.sh. It looks like dartanalyzer doesn't see package:parsers/parsers.dart and package:test/test.dart when analyzing test/parsers_test.dart. I suppose something has changed in the behavior of dartanalyzer and the way it resolves files. Would you happen to know anything about that?

[polux]

I'm still trying to digest all the changes but I fail to understand the need for this helper method: why do we need special cases for String, List, num, int and double? Why isn't return orginal as C enough?

[devkabiir]

Because of nested types. The compiler was not accepting say Parser<List<dynamic>> as Parser<List<String>>, even Parser<dynamic> as Parser<String> doesn't work, when I started working on it. But since then I've added a lot types. Few places might still have dynamic for input or output. Plus this method also helped migrate our parser with little efforts.

[devkabiir]

At the time thought I needed it for all dart primitives. After testing locally I seems we don't need it for any primitives now. Only a null check and original as C;

[polux]

Thanks! Why do we need the null check for?

[devkabiir]

Because of failed parses. They return a ParseResult.failure() which doesn't have a type by it's nature. But I added a generic type to it too and made it a static method so it can have generic types. Now if we don;t specify the type while creating a failure ParseResult it will become ParseResult<dynamic> and when dynamic is introduced the type system goes nuts. So if C is Null in case of a Parser<Null> or if original == null in case of a null result. We don;t care about the types we just return null. Also null can't be type casted to anything else. But null can take place of anything else. null as String will always have type Null so if a thing that called cast<String>(something) was expecting a value of type String but got a value of type Null instead it will freak out at runtime. But that thing will happily accept a String of value null

[devkabiir]

I hope I didn't make it even more confusing.

[polux]

Ok, I see what's happening (at least in some cases). In copy I'm using a null value as a way to mark it wasn't set by the caller. But in some cases (like in skipManyUntil) I genuinely want the value to be null because the parser is of type Parser<Null>. Yet in that case this line:

cast<B>(value ?? this.value),

evaluates to:

cast<B>(this.value)

which is not the intended behavior. This causes errors like type 'String' is not a subtype of type 'Null' in type cast. The remedy is to have a default value other than null for the value parameter. Some singleton value (or an "Undefined" class for instance) that we can quickly check against using pointer equality.

I'm happy to merge your PR and work from there. But if you prefer we can also continue this code review and you can do the changes.

[polux]

Ahah, the singleton trick won't work because it can't be of type B. I guess it's just easier to have two copy methods: one for when there's a new value and one for when there isn't.

[devkabiir]

I see, I tried the exact same approach in my decorator package in copyWith method. But switched to using a bool flag for being explicit that the caller wants it to be null. We could do the same here?
So it would become nullValue ? null : value ?? this.value.
Edit: And the caller would do copy(....value: myValue, nullValue: mayValue == null). In case they aren't sure it's gonna be null or not. I know this increases the efforts, but solves the type issues and help being more explicit which in turn helps the next programmer.

[devkabiir]

I don't know how to fix it without running Dart 2.3+ and it isn't easy to switch versions on windows. From the stacktrace, It's definitely not a parsers lib issue. I added more types and replaced the operators with method equivalents. That should make it work for you.

About the analyzer. I ran it locally for individual files. And on directories too. it worked fine. Except for a deprecation warning on a test. The analyzer will ignore any files present in the exclude key of analysis_options.yaml Do you have anything added to it locally.

[devkabiir]

It is the other way around. Silly me. Can you look at or method? and the | operator. The way it is now 'works', But I wasn't sure a the time if this refactoring was required. Say
Parser<B> or<B extends A>(Parser<B> otherParser) might be intended. But in practical scenario otherParser don't always extend the original one.

[polux]

The static type returned by a union of two parsers should be their most common ancestor. So it should be something like:

Parser<C> or<A extends C, B extends C>(Parser<B> p)

Which can probably be simplified into:

Parser<B> or<A extends B>(Parser<B> p)

since generics are covariant in dart. (At least they were in 1.0, maybe this has been fixed in 2.0? In doubt I wouldn't use the simplified version.)

[devkabiir]

What do you think about letting the caller decide? The caller would always know better, no? This opens up the possibility of otherParser not being of same base type in which case or always returns Parser<dynamic>.
Parser<O> or<O>(Parser otherParser)
If they don't specify its dynamic by default.
If they do it gets casted.
e.g.

final c = char('a').or<String>(char('b'));
/// or
final Parser<String> c = char('a').or(char('b'));
print(c.runtimeType); /// will print `Parser<String>`
print(c is Parser<String>); /// true
print(c as Parser<String>); /// works

Another example would be

Parser<RegEx> myRegExParser;
final c = char('a').or<Pattern>(myRegExParser);
/// c is of base type `Pattern`

In case of types with no common base type

Parser<ClassA> intParser;/// parses `'1'` to `1`
Parser<ClassB> stringParser; /// parser `'one'` to `1`
final c = stringParser.or(intParser);
/// Here [c]'s result will always be an `int`

[polux]

I'm not sure I follow: wouldn't the caller also decide with Parser<C> or<A extends C, B extends C>(Parser<B> p)? The caller could chose to set C to dynamic if that's truly the closest common supertype.

[devkabiir]

Umm, When you say A extends C which A are you referring to? In this
Parser<C> or<A extends C, B extends C, C>(Parser<B> otherParser)
A is not equal to the original A used when declaring class Parser<A> {...}. Unless I'm mistaken. Also you'd have to specify all three A,B,C types when calling or if a typed parser is desired. Otherwise omit all three and get Parser<dynamic>. Also I don't think C can be set to dynamic since nothing can extend dynamic

[polux]

In Parser<C> or<A extends C, B extends C, C>(Parser<B> otherParser) the A is a new A. That's my understanding.

You're correct. What I was aiming at was:

Parser<C> or<B extends C, C super A>(Parser<B> otherParser)

But that doesn't seem to exist. The spec says:

Dart does not support lower bounds for type parameters".

Too bad! (I found a rendered version of the spec by the way: https://www.dartlang.org/guides/language/specifications/DartLangSpec-v2.2.pdf).

Anyway, the spec also says:

17 Class Covariance: A parameterized type based on a generic class C is a subtype of a parameterized type based on the same class C if each actual type argument of the former is a subtype of the corresponding actual type argument of the latter.

So generics are still covariant in Dart 2.0. In the case of parsers I think this is sound: they actually are covariant. So we could use this signature:

Parser<B> or<B super A>(Parser<B> p)

Unfortunately as we'be established lower bounds don't exist in Dart. So I guess we're left with:

Parser<A> or(Parser<A> p)

Let's say S is a subtype of T. Then by covariance Parser<S> is a subtype of Parser<T>. In p1.or(p2) if p1 is of type Parser<T> and p2 is of type Parser<S> then the typechecker wouldn't complain since p2 is also of type Parser<T> by subtyping. If p1 is of type Parser<S> and p2 is of type Parser<T> then the caller would have to explicitly cast p1:

(p1 as Parser<T>).or(p2)

Given that in most cases the types should be the same that seems ok to me. What do you think? If you still prefer the "let the user chose" option could you please elaborate on what the signature would look like and where the cast would happen?

[devkabiir]

17 Class Covariance: A parameterized type based on a generic class C is a subtype of a parameterized type based on the same class C if each actual type argument of the former is a subtype of the corresponding actual type argument of the latter.

I don't fully understand this 😅.

Given that in most cases the types should be the same that seems ok to me. What do you think?

In case they aren't castable to one another there isn't much dart can do anyway, other than Parser<dynamic>. This is a good approach.

Parser<A> or(Parser<A> p)

This is acceptable. And a win for the | operator since it removes the generic type from or.

If you still prefer the "let the user chose" option could you please elaborate on what the signature would look like and where the cast would happen?

Like so, in this case we don't care for the specificity of otherParser. This gives choice to the caller. If they provide O then it gets casted. And Dart's type system can also
infer it from somewhere else so the caller doesn't always have to provide the type.

  Parser<O> or<O>(Parser otherParser) {
    ParseResult<O> parserFunction(String s, Position pos) {
      final res = run(s, pos);
      if (res.isSuccess || res.isCommitted) {
        return res.copy<O>(value: cast<O>(res.value));
      } else {
        final res2 = p.run(s, pos);
        return res2.copy<O>(
            expectations: res.expectations.best(res2.expectations));
      }
    }

    return Parser<O>(parserFunction);
  }

Unfortunately the | would then always return Parser<dynamic>. Which is why I would go for Parser<A> or(Parser<A> p) but not sure how that would play with current usage of or throughout the lib, example, test, etc.

[polux]

I don't fully understand this .
It's just a fancy way of saying that List is considered a subtype of List by Dart. It's unfortunately sometimes unsound but that's something dart decided to live with (and rely on runtime checks instead). For example the following program typechecks and fails at runtime:

class Fruit {}
class Apple extends Fruit {}
class Banana extends Fruit {}

main() {
  List<Apple> apples = new List<Apple>();
  List<Fruit> fruits = apples;
  fruits.add(new Banana());
}

Fortunately for us in the case of parsers it is correct that a Parser is a Parser because parsers are "read only" unlike lists.

This is acceptable. And a win for the | operator since it removes the generic type from or.

Good point about the | operator.

Like so [...]

I see. I generally would like to avoid casts in hot spots of the library. Especially if these casts inspect the generic argument of a function. I don't have proof but I'm pretty certain these types of casts are slower.

but not sure how that would play with current usage of or throughout the lib, example, test, etc.

I guess we need to test it :)

[devkabiir]

Upon further thinking

Parser or(Parser p)

In this case p would have to be a covariant of A, right?
Since there is no generic type parameter, How is the caller supposed use or when p is not a covariant of A?

In case of and we return ParserAccumulatorN. How about doing something similar to or?

[devkabiir]

@polux
I'm curious to know your thoughts on https://medium.com/dartlang/dart-declaration-site-variance-5c0e9c5f18a5
Could this be used here?

[elmcrest]

Hey @polux, @devkabiir what's the state of this?

[devkabiir]

It's been a while since I last worked on it, If @polux is available for review, I can give it another run.

[polux]

Hi, by all means, please give it another run. I'll do my best to spend some time reviewing it.