-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[no-object-literal-type-assertion] False positive when asserting function argument #66
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
Comments
I'm curious why you think
and your example demonstrates unsafe code which will break at runtime. context: I merged the original no-object-literal-type-assertion rule into TSLint core, and I'm interested in understanding the direction of this project, especially any differences in rule semantics. |
Ah, sorry, I missed the interface Foo {
foo: number;
}
function print(x: Foo): void {
console.log(x.foo)
}
print({} as Foo) // excess properties would be banned by the compiler here
^^^^^^^^^ [no-object-literal-type-assertion] |
Honestly I don't see why linter should be bothering with what's Typescript doing just fine by itself? What would you do if it's a more complex scenario such as below? interface Foo {
foo: number
}
type Z<T> = Partial<T>
print({} as Z<Foo>) Simply enabling |
I'd have to test tslint in this case, as this rule is intended to be a like-for-like reimplementation of that logic. Though I suspect that this is an implementation bug.
This plugin exists to complement the compiler, to enforce some conventions on code that is probably 100% valid. One could argue that your original code has a code smell, because it is passing more than the function signature is asking for - breaking its contract. I.e. consider this example: interface Foo {
foo?: number;
}
function apiReq(x: Foo): void {
Object.keys(x).forEach(key => sendKVPToAPI(key, x[key]))
}
apiReq({ bar: 5 } as Foo) This code is syntactically valid in TS, but doesn't mean it's semantically correct. When this runs, you'll send invalid data to the API, because you've forced some behaviour via using the Whilst of course this is an exaggerated case, these things do happen, so you can see why people might want a lint rule to catch and help to prevent people using compile time correct code. The above case is why flow has its "exact object" syntax, to enforce a stricter (unbreakable) type contract. type Foo = {| // the |'s make it an exact type
foo?: number;
|}
function apiReq(x: Foo): void {
Object.keys(x).forEach(key => sendKVPToAPI(key, x[key]))
}
apiReq(
({ bar: 5 }: Foo), // yes, this is flow's type cast syntax
// ^ Cannot cast object literal to `Foo` because property `bar` is missing in `Foo`, but exists in object literal.
) |
Over the years we've generally found that there will be some amount of overlap between the linter & compiler checks; it's not a black & white distinction (palantir/tslint#4100 is just one example). I agree with @bradzacher's stated goal for this plugin: "This plugin exists to complement the compiler, to enforce some conventions on code that is probably 100% valid." The TS type system is very flexible & gradual and this is good for incremental adoption, but for users who want to coerce the system to be more static & strict, we have lint rules.
IMO Partials are a TS code smell, they throw away some type safety and should be used sparingly (and therefore the lint rule shouldn't be designed to suit them as a standard code pattern, but rather an edge case). That is an opinionated stance. Either way, the code in that example doesn't compile, you get this error with TS 3.1.2:
In cases where the compiler produces an error, I think it's ok for the linter to also produce an error (at least, I don't think it matters much in practice).
|
Actually I'd really like if It would be nice if we could make this compatible with the TSLint version but also add some pragmatic flags to make it so that we could use it all the time without exceptions.
|
I'm of the opinion that you can never have enough options! I suspect that it should by default only flag |
type assertion and type casing is awesome during migration of code, but when you finish you want to avoid using it, otherwise there is no point in using typescript if you just cas everything to incompatible types. adding option is great idea (more ways to configure = better), but by default it should just warn about all of usages... |
I generally agree it's code smell, but my original example is of course a trimmed down version of the actual code. If I remember correctly the original problem was using external API for which I have no control (JSON updates to Objection.js models): const client = await ClientModel.query().where(/* ... */).first()
// ...
await client.$query().patch({ "data:subfield": foo } as PartialUpdate<ClientModel>) (I'm not entirely sure it was it, but I'm definitely using I do however want to keep that rule for its original and documented purpose (prevent developers from static cast in assignments). On the other hand I realise I could always use |
@IlyaSemenov you should use something like this in this case await client.$query().patch<PartialUpdate<ClientModel>>({ "data:subfield": foo }) where patch in template/geenric function class query {
patch<T extends PartialUpdate>( model: T) {}
} |
As I have told, it's an external API which I'm not designing, so I don't see how your suggestion is applicable here. |
@IlyaSemenov you can augment definition file // module-name.d.ts
declare module 'module-name/path/test' {
export class query {
patch<T extends PartialUpdate>(model: T): Promise<T> // this is just example
}
} |
@armano2 I am well aware of augmenting external modules, thank you. In this particular case the typings already make sense: type PartialUpdate<QM extends Model> = {
[P in keyof QM]?: QM[P] | Raw | Reference | QueryBuilder<any, any[]>
};
interface QueryBuilderBase<QM extends Model, RM, RV> extends QueryInterface<QM, RM, RV> {
patch(modelOrObject: PartialUpdate<QM>): QueryBuilderYieldingCount<QM, RM>;
} Extending the interface for In either case, let's not do Typescript learning sessions here, this issue is about something completely different and is irrelevant to my comprehension of TS and/or possibility of other workarounds. |
now i see full context 👍 |
TypeScript have added support for |
Repro
Expected Result
No errors.
Actual Result
Additional Info
The primary purpose of the rule (as I see it) is to prevent object assignment such as
const x = { ... } as T;
which is better rewritten asconst x: T = { ...};
, but the rule disallows all other legitimate uses.Versions
eslint-plugin-typescript
1.0.0-rc.2
typescript-eslint-parser
21.0.2
typescript
3.2.2
The text was updated successfully, but these errors were encountered: