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

[Bug]: Queue<DataTypeOrJob> wrong variance #2951

Open
1 task done
jeengbe opened this issue Dec 4, 2024 · 2 comments
Open
1 task done

[Bug]: Queue<DataTypeOrJob> wrong variance #2951

jeengbe opened this issue Dec 4, 2024 · 2 comments
Labels
enhancement New feature or request PR Request

Comments

@jeengbe
Copy link

jeengbe commented Dec 4, 2024

Version

v5.31.0

Platform

NodeJS

What happened?

I have only a very shallow understanding of variance yet, please do correct me if I use terminology incorrectly. I believe the issue holds regardless of what you name the factors involved, though.

The Queue class should be contravariant on DataOrJobType, as it's an input type. The generated types however allow it to be used contravariantly, leading to unsafe code.

Should be as simple as adding an in keyword to the type parameter here. If you modify your local .d.ts file accordingly, the below reproduction correctly errors. I'm not fully sure whether to give it the in tag only or whether it's invariant with in out.

How to reproduce.

import { Queue } from 'bullmq';

async function sendNumberToQueue(queue: Queue<string | number>) {
  await queue.add('job', 1);
}

declare const stringQueue: Queue<string>;

void sendNumberToQueue(stringQueue);

Should error for stringQueue is only aware about string messages.

Relevant log output

No response

Code of Conduct

  • I agree to follow this project's Code of Conduct
@jeengbe jeengbe added the bug Something isn't working label Dec 4, 2024
@manast
Copy link
Contributor

manast commented Dec 4, 2024

As I am not an expert on typescript generics and less so on variance and contra variance I asked ChatGPT o1:
Variance in TypeScript Generics

Variance Types:

•	Covariant (out): Allows a type to be substituted with its subtype.
•	Contravariant (in): Allows a type to be substituted with its supertype.
•	Invariant: No substitution is allowed; types must match exactly.
•	Bivariant: Allows both covariance and contravariance (unsafe in many cases).

TypeScript’s Default Behavior:

•	Type Parameters: By default, TypeScript treats type parameters as bivariant in classes and interfaces unless strict checks are enforced.
•	Function Parameters: With strictFunctionTypes enabled, function parameter types are checked for correct variance.

Enforcing Correct Variance in Queue

To prevent unsafe assignments, we need to make DataTypeOrJob invariant or contravariant in the Queue class.

Why Contravariant?

•	Input Position: The Queue class consumes DataTypeOrJob (e.g., in the add method), so it should be contravariant on this type parameter.

However, TypeScript does not currently support variance annotations (in or out keywords) on type parameters. Instead, variance is inferred based on how the type parameters are used within the class.

Solution: Adjusting the Queue Class to Enforce Invariance

To make DataTypeOrJob invariant, we need to ensure it’s used in both input and output positions within the Queue class. This forces TypeScript to treat the type parameter as invariant.

Step 1: Use DataTypeOrJob in Output Positions

Add a method to the Queue class that returns a value of type DataTypeOrJob.

Example:

export class Queue<DataTypeOrJob = any> {
  // Existing methods

  // Add a method that returns DataTypeOrJob
  getData(): DataTypeOrJob {
    // Implementation not important for type checking
    throw new Error('Not implemented');
  }
}

Step 2: Ensure DataTypeOrJob is Used in Input Positions
This is already the case with methods like add:

add(data: DataTypeOrJob): void {
  // Implementation
}

Result:

By using DataTypeOrJob in both input and output positions, TypeScript treats it as invariant. This means that Queue is no longer assignable to Queue<string | number>, and the unsafe assignment in your example will produce a compile-time error.

Testing the Adjusted Queue Class

export class Queue<DataTypeOrJob = any> {
  add(data: DataTypeOrJob): void {
    // Implementation
  }

  getData(): DataTypeOrJob {
    // Implementation
    throw new Error('Not implemented');
  }
}

// Your example
async function sendNumberToQueue(queue: Queue<string | number>) {
  await queue.add(1);
}

declare const stringQueue: Queue<string>;

void sendNumberToQueue(stringQueue);

Compiler Error:

Argument of type 'Queue<string>' is not assignable to parameter of type 'Queue<string | number>'.
  Types of property 'getData' are incompatible.
    Type 'string' is not assignable to type 'string | number'.

Explanation

•	Type Incompatibility: Queue<string> cannot be assigned to Queue<string | number> because getData() returns string, which is not assignable to string | number in this context.
•	Variance Enforcement: By making DataTypeOrJob invariant, TypeScript correctly prevents the unsafe assignment.

Alternate Approach: Using TypeScript’s strictFunctionTypes

Ensure that your tsconfig.json has the following setting:

{
  "compilerOptions": {
    "strict": true,
    "strictFunctionTypes": true
  }
}

@manast manast added enhancement New feature or request PR Request and removed bug Something isn't working labels Dec 4, 2024
@jeengbe
Copy link
Author

jeengbe commented Dec 5, 2024

It's strange o1 doesn't know about variance annotations even though they were clearly added more than a year before its knowledge cutoff (in TS 4.7, 2022). I have also not seen it be that wrong before, the proposed still doesn't work

image

Note the in out annotation in the second block


I would be open to put out a small MR for adding variance annotations, but that would prevent anyone from using TS <4.7 from upgrading to the latest version of this package. But perhaps that's a good thing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request PR Request
Projects
None yet
Development

No branches or pull requests

2 participants