temperlang / temper

3 stars 0 forks source link

Unhelpful subtype check #152

Open tjpalmer opened 3 months ago

tjpalmer commented 3 months ago

Here's example code from temper-regex-parser that uses a cast to ensure we see List<Pair<String, RegexNode>> rather than List<Pair<String, RegexNode>>:

        new Map([
          // Make one explicit simple string just for demo.
          new Pair("char-a", new CodePoints("a").as<RegexNode>()),
          new Pair("char-b", parseWith(
            "(?$char-b)",
            new Map([ new Pair("char-b", parse("b")) ]),
          )),
          new Pair("char-c", parse("(?cee=c)")),
          new Pair("char-d", parse("(d)")),
          new Pair("char-e", parse("(e|e)")),
          new Pair("char-f", parse("[f-f]")),
        ]),

But we now get this error when building that we didn't used to get:

33: new Pair("char-a", new CodePoints("a").as<RegexNode>()),
                       ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
[-work//tests/variations.temper+924-959]@G: Unnecessary type check to RegexNode from expression with type CodePoints which is a subtype

Used to be error free. And without that cast, we get even more errors:

   ┏━━━━━━━━┓
31:┃new Map([
32:┃  // Make one explicit simple string just for demo.
   ┃⋮
41:┃  new Pair("char-f", parse("[f-f]")),
42:┃]),
   ┗┛
[-work//tests/variations.temper+833-1277]@G: Expected class or interface type, not Pair<String, CodePoints> | Pair<String, RegexNode>
   ┏━━━━━━━━┓
31:┃new Map([
32:┃  // Make one explicit simple string just for demo.
   ┃⋮
41:┃  new Pair("char-f", parse("[f-f]")),
42:┃]),
   ┗┛
[-work//tests/variations.temper+833-1277]@G: Expected subtype of List<Pair<K__24, V__25>>, but got List<Pair<String, CodePoints> | Pair<String, RegexNode>>
   ┏┓
31:┃new Map([
32:┃  // Make one explicit simple string just for demo.
   ┃⋮
41:┃  new Pair("char-f", parse("[f-f]")),
42:┃]),
   ┗━┛
[-work//tests/variations.temper+825-1278]: Cannot translate Invalid type: ❎

In both cases, all tests pass on working backends (currently just csharp, js, & lua).

I can try this alternative:

        new Map([
          // Make one explicit simple string just for demo.
          new Pair("char-a", new CodePoints("a")),
          new Pair("char-b", parseWith(
            "(?$char-b)",
            new Map([ new Pair("char-b", parse("b")) ]),
          )),
          new Pair("char-c", parse("(?cee=c)")),
          new Pair("char-d", parse("(d)")),
          new Pair("char-e", parse("(e|e)")),
          new Pair("char-f", parse("[f-f]")),
        ].as<List<Pair<String, RegexNode>>>()),

But it still has an error:

   ┏━━━━━━━━┓
31:┃new Map([
32:┃  // Make one explicit simple string just for demo.
   ┃⋮
41:┃  new Pair("char-f", parse("[f-f]")),
42:┃].as<List<Pair<String, RegexNode>>>()),
   ┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
[-work//tests/variations.temper+833-1313]@G: Unnecessary type check to List<Pair<String, RegexNode>> from expression with type List<Pair<String, CodePoints>
 | Pair<String, RegexNode>> which is a subtype

Casting the entire map has errors also.

Or in other words, I don't know a way to avoid error messages now on something that used to work.

We could either be more clever went to present the new error or else remove the error or else move sooner in avoiding "or" types or else improved unifying of types in generic collections or such like.

tjpalmer commented 3 months ago

Personally, I think I vote for just removing this warning/error message for now, or else put it on "fine" logging level or something.

tjpalmer commented 3 months ago

I'm not sure if we should elide the as<>() or if it's still helpful for any backends in odd cases. But that could be a separate PR from the one that reduces the log level for this message.

mikesamuel commented 3 months ago

I can turn it off. I imagine we could redo it as a linter thing if it turns out to have no effect on type inference.

tjpalmer commented 3 months ago

I got some cases where the cast is to the same type. We probably still can error/warn on this case but maybe with different text:

95: var regex = base.as<Sequence>();
                ┗━━━━━━━━━━━━━━━━━┛
[-work//tostring.temper:95+18-37]@G: Unnecessary type check to Sequence from expression with type Sequence which is a subtype