Support new Flow "opaque type" syntax

[lll000111]

https://flow.org/en/docs/types/opaque-types/

Right now this new Flow 0.51 syntax produces eslint errors.

[johnhaley81]

I don't think this is a error with this package but instead this plugin needs babel/babel#6081 to land first.

[bradennapier]

Its merged into v7 of babel and still produces errors. Any chance to get it working with v7 as I would imagine it would trickle down when the backport is complete?

[rkurbatov]

Yeah, it seems the only thing that doesn't allow us to play with long-awaited opaques.

[bradennapier]

@johnhaley81 looks like the backport is also merged now

[valscion]

Ok, now with [email protected] and [email protected], we're in a position that no longer raises syntax errors.

However, one issue still remains: Those opaque types still raise no-undef related errors. Could it be that use-flow-type rule should be fixed here to get rid of those errors?

I tried to add some tests to see a failure in the current state:

diff --git a/tests/rules/assertions/useFlowType.js b/tests/rules/assertions/useFlowType.js
index cccdaba..00d439e 100644
--- a/tests/rules/assertions/useFlowType.js
+++ b/tests/rules/assertions/useFlowType.js
@@ -90,6 +90,12 @@ const VALID_WITH_USE_FLOW_TYPE = [
     errors: [
       '\'A\' is defined but never used.'
     ]
+  },
+  {
+    code: 'opaque type A = Y',
+    errors: [
+      '\'A\' is defined but never used.'
+    ]
   }
 ];
 
@@ -120,6 +126,7 @@ const ALWAYS_VALID = [
   'type A = Y; function x(a: A) { a() }; x()',
   'function x(a: A) { a() }; type A = Y; x()',
   'type A = Y; (x: A)',
+  'opaque type A = Y; (x: A)',
   '(x: A); type A = Y',
   'function x<A>(): A {}; x()',
   'import type A from "a"; (function(): A {})',

...but I don't really know how to digest this test failure? Any help would be appreciated — I'll happily try to fix this error if it is possible to do so with this plugin.

[bradennapier]

Anyone? opaque types are definitely key and not having them sucks :( I would look into it but don't currently have the time (on a deadline).

One kind of weird thing I realized when trying it is that it doesn't always happen?

[bradennapier]

@valscion - interesting - i just figured out why only some seem to raise the error. It is if the opaque type is not used in the file. If it is used in the same file it works fine.

opaque type Foo = string;

vs

opaque type Foo = string;

export const foo: Foo = 'MY_OPAQUENESS';

The latter does not raise the errors. While it actually kind of makes sense that this would be an error (if you export an opaque type but provide no way of retrieving or working with it then it can never really be used anyway), it should probably be its own specific error in this case. Something like "no-useless-opaque"

I don't know enough about how opaque types actually work yet as I haven't had a chance to use them. I am not sure if providing a type to the opaque type would make it non-useless etc but the rule def should be there I would think!

[valscion]

I'd love to get a closer look on this. I'd need to get some pointers on where to start looking, though, and what kind of test would I need to build to get a meaningful error message out. I've never seen tests like this plugin has, so I'm left a bit clueless.

@valscion - interesting - i just figured out why only some seem to raise the error. It is if the opaque type is not used in the file. If it is used in the same file it works fine.

Huh, that's a nice tip. So basically opaque types "work", it is just that they raise strange error (no-undef) instead of a more meaningful one in case they're unused (e.g. no-unused-vars or adding the new no-useless-opaque).

---

I think we'd need to first get incorrect no-undef errors away. It seems that my previous test change attempts incorrectly targeted no-unused-vars, but the error that we're seeing is actually no-undef being false negative, not no-unused-vars 🤔

Let's see if I can come up with something...

[bradennapier]

Yeah it's actually probably going to be a little bit more complex than previously thought. I successfully integrated opaque types into my project and first of all -- game changer -- absolutely amazing to have.

I have an example of them all in use that could be used as a basis here:

https://github.com/Dash-OS/flow-type-transformer/tree/master/tests/flow/opaque-transformation

these do not have any errors for me currently with eslint

So defining an opaque type as previously shown will give us the error:

opaque type MyType = {
  one: 'two',
};

The more complex problem will come in when trying to actually determine if an opaque type is "useless". In order for an opaque type to ever really match it needs to be returned by a function within the same file that it is defined or exported as a value directly.

At the moment this actually will pass eslint tests:

export opaque type Dash$ResponseWrapper =
  | MyType
  | Dash$HandshakeResponse
  | Dash$SuccessWrapper
  | Dash$ErrorWrapper;

opaque type MyType = {
  one: 'two',
};

But the MyType is actually still "useless" because it can never really match anything since I don't provide it to the user through any specific means.

The follow methods should make an opaque type legal (among others I am sure, identifying the various ways an opaque type can be provided to the user will be the difficult part I think):

// now b is an opaque type and that value is the only way to use the opaque type
export const b: MyType = {
  one: 'two',
};

// by exporting the type through a function call
export function getMyType(): MyType {
  return ({
    one: 'two',
  }: MyType);
}

What I am not sure about currently, but I suspect it is the case, is if something like this allows opaque types to be used. If so, then it is going to be significantly harder to track and handle whether an opaque type is actually "useless".

const Value: Map<*, *> = new Map();

export function one() {
  Value.set('mytype', ({ one: 'two' }: MyType));
}

export function two() {
  return Value;
}

---

It stands to reason that a few rules could probably be looked into:

• no-unused-opaque - Simply checks if an opaque is defined but never used anywhere. This is pretty much how it works currently but it reports no-undef instead.
• no-useless-opaque - Checks if an opaque type is created and either never used or exported. If exported, there must be some way of getting the opaque type.
• no-local-opaque - Requires that the opaque type be exported. This won't always be the desired effect since there may be times the opaque type will be checked internally but provided in some other way (the user wont be able to check it but internally we still can).
• require-opaque-getter - For example, if an opaque type is provided, then it would be expected that at least one exported function will directly return that type as a value. Perhaps as a union returning multiple opaque types or directly or some other means.

export opaque type MyType = {
  one: 'two',
};

export function getMyType(): MyType {
  return ({
    one: 'two',
  }: MyType);
}

Anyway - seems like it could get pretty specific here. Just some ideas. I am seeing the power of opaque types now though and its definitely very exciting. It's already caught 3-4 bugs that have been in our backend API for 3+ years without us knowing.

[valscion]

Sounds amazing! Would you be willing to move your tremendous comment to a new issue, so we could focus on the new rule addition separately from the problem with incorrect no-undef warnings?

I'd love if this issue could be fixed by fixing the false negative. Then later, implementing a strategy for catching false positives, like with the new rule, could be figured out separately from this issue.