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

"type MyType = MyType" should not be allowed #5771

Closed
massimonewsuk opened this issue Feb 5, 2018 · 7 comments
Closed

"type MyType = MyType" should not be allowed #5771

massimonewsuk opened this issue Feb 5, 2018 · 7 comments
Labels

Comments

@massimonewsuk
Copy link

massimonewsuk commented Feb 5, 2018

type JsonField = JsonField; // Why is this allowed? JsonField becomes defined as "any".
type JsonField2 = SomeNonExistentType; // This gives an error - as expected

type DatabaseRow = {
    id: number;
    dumping_ground: JsonField | null;
}

type Entity = {
    id: number;
    attributes?: {
        [key: string]: string;
    };
}

function mapEntity(row: DatabaseRow): Entity {
    const entity: Entity = {
        id: row.id
    };
    if (row.dumping_ground !== null) {
        entity.attributes = row.dumping_ground.attributes;
    }
    return entity;
}

So I came across this one because the field DatabaseRow.dumping_ground is inspired by a postgres nullable JSONB field that could contain anything, if defined. Unfortunately we have the following in our .flowconfig so I can't use any | null;

[lints]
all=error

So instead of doing // $FlowFixMe everywhere I wanted to do any for JSON, I decided to encapsulate it as type JsonField = any;, so that I could put the // $FlowFixMe in one place. I then did a find-replace on any in the file (by accident turning the above declaration into type JsonField = JsonField, and much to my surprise Flow then told me that my // $FlowFixMe wasn't needed! I took it away and saw that indeed Flow does not complain about type JsonField = JsonField;

So thanks for sorting my dilemma cause this bug means I don't have to spend any time trying to find out how else to specify any because our linting doesn't allow it.

However, it is a bug and should probably be fixed. Copying/pasting the above code into the TypeScript playground gives an error saying that the type JsonField is a circular reference.

Keep in mind that circular type references are not always bad. For example the following is a perfectly valid example of the usefulness of circular type references. You could define a dynamic JSON import as JSONValue using the code below, and then do useful type-narrowing on it to get useful errors etc.

type JSONPrimitive = string | number | boolean | null;
type JSONObject = { [member: string]: JSONValue };
type JSONArray = Array<JSONArray | JSONValue | JSONObject>;
type JSONValue = JSONPrimitive | JSONObject | JSONArray;

function doSomething(x: JSONValue) {
    printNotNull(x);
    if (typeof x === "string") {
        printString(x);
        printNumber(x);
        printBoolean(x);
        printObject(x);
        printArray(x);
        printNotNull(x);
    }
    if (typeof x === "number") {
        printString(x);
        printNumber(x);
        printBoolean(x);
        printObject(x);
        printArray(x);
        printNotNull(x);
    }
    if (typeof x === "boolean") {
        printString(x);
        printNumber(x);
        printBoolean(x);
        printObject(x);
        printArray(x);
        printNotNull(x);
    }
    if (typeof x === "object" && x !== null && !Array.isArray(x)) {
        printString(x);
        printNumber(x);
        printBoolean(x);
        printObject(x);
        printArray(x);
        printNotNull(x);
    }
    if (Array.isArray(x)) {
        printString(x);
        printNumber(x);
        printBoolean(x);
        printObject(x);
        printArray(x);
        printNotNull(x);
    }
}

function printString(x: string) {
}

function printNumber(x: number) {
}

function printBoolean(x: boolean) {
}

function printObject(x: JSONObject) {
}

function printArray(x: JSONArray) {
}

function printNotNull(x: string | number | boolean | JSONObject | JSONArray) {
}

The 4 lines of code at the top that define the types would fail if pasted into the TypeScript playground as-is because it's detected as a circular type reference (microsoft/TypeScript#14174). But this check can be bypassed in TS using the following code:

type JSONPrimitive = string | number | boolean | null;
type JSONObject = { [member: string]: JSONValue };
interface JSONArray extends Array<JSONArray | JSONValue | JSONObject> { };
type JSONValue = JSONPrimitive | JSONObject | JSONArray;

Interestingly, doing that in Flow causes the Array type checking to completely break down, which I'll raise separately.

@massimonewsuk
Copy link
Author

Still an issue in v0.67.0

@massimonewsuk
Copy link
Author

Still an issue in v0.70.0

@vkurchatkin
Copy link
Contributor

This is not really a bug, I would say that it works as expected.

@massimonewsuk
Copy link
Author

massimonewsuk commented Apr 23, 2018

So you're saying this is valid?
type MyType = MyType;

Bear in mind that MyType isn't declared anywhere else

@vkurchatkin
Copy link
Contributor

So you're saying this is valid?

Yes, this is allowed because recursive types are allowed.

@mrkev
Copy link
Contributor

mrkev commented Apr 23, 2018

I agree, yet IMO we should still complain about recursive types which doesn't have a base case like OCaml does

@SamChou19815
Copy link
Contributor

This is now banned

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants