-
Notifications
You must be signed in to change notification settings - Fork 12.8k
add excess property checks to spread objects defined inline #59277
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
base: main
Are you sure you want to change the base?
Conversation
@typescript-bot test it |
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
@iisaduan Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@iisaduan Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
@iisaduan Here are some more interesting changes from running the top 400 repos suite Details
|
@typescript-bot test top400 |
@iisaduan Here are the results of running the user tests with tsc comparing Something interesting changed - please have a look. Details
|
@iisaduan Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
User Test analysisazure-sdk
This might be a param that was not generated correctly. It might be the case that defining import { GeneratedClientOptionalParams } from './generated';
const internalPipelineOptions: GeneratedClientOptionalParams = {
...this.clientOptions,
...{
apiVersion: this.apiVersion,
loggingOptions: {
additionalAllowedHeaderNames: this.clientOptions?.loggingOptions?.additionalAllowedHeaderNames,
additionalAllowedQueryParameters: this.clientOptions?.loggingOptions?.additionalAllowedQueryParameters,
logger: logger.info,
},
},
...(isTokenCredential(this.credential)
? {
credential: this.credential,
credentialScopes: ["https://webpubsub.azure.com/.default"],
}
: {}),
}; |
Top 400 analysisdubinc/dub and noodle-run/noodleThese are errors on the same FaridSafi/react-native-gifted-chatThis is on an element in function Composer(): React.ReactElement {
return (
<TextInput
style={[
styles.textInput,
textInputStyle,
{ // error
height: composerHeight,
...Platform.select({
web: {
outlineWidth: 0,
outlineColor: 'transparent',
outlineOffset: 0,
},
}),
},
]}
{...textInputProps}
/>
)
} labring/FastGPTexcess property on the value a record object called vscodeExcess properties in types used for options:
radix-ui/primativesEach of these are in react-bootstrap/react-bootstrapExcess property in a style prop. headlessuiExcess property an object that is a function param, and the function uses the object as |
Changed some behavior (specifically: decided on points 2 and 3 in the PR description) based on the test results above. @typescript-bot test top400 |
Hey @iisaduan, the results of running the DT tests are ready. Everything looks the same! |
@iisaduan Here are the results of running the user tests with tsc comparing Everything looks good! |
@iisaduan Here they are:
tscComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
tsserverComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
startupComparison Report - baseline..pr
System info unknown
Hosts
Scenarios
Developer Information: |
@iisaduan Here are the results of running the top 400 repos with tsc comparing Something interesting changed - please have a look. Details
|
@iisaduan |
Glad to hear that this would be useful! If there are other cases that you'd like to have checked, please let me know, and I can look into it when I start up on this again. The current status of this PR is that the check is inconsistent because of the way we get properties from union types, so some simple/reasonable cases that should be checked don't get checked. Example: const definedObj = { a: 1 } ;
const x: { a: 1 } = { ...( someBool ? definedObj : { b: 1 }) } // should error on `b`, but currently does not In some cases, the order of the property definitions in the object will change whether it's checked or not. I spent some time looking into other ways to resolve property definitions (that also isn't slow), but I haven't had the chance to follow through completely |
I've had bugs in production due to this longstanding TypeScript issue. I'm really excited to see where this will go! |
Fixes #39998
This PR adds EPC to a property
x
from spread objects ifx
is defined is defined inlineHasExpressionInitializer
or a return statementJsxOpeningLikeElement
Examples
1- Checks only occur on spreads defined inline. Mostly, this means
x
is not defined in a variable that is being spread.We now also check conditional spreads if defined inline:
The tests also show that this check works with
satisfies
.2-
3- This check is blocked when the parent object is within a JSX element. For example, a case that was hit in an earlier iteration of this PR had an error in an object spread and defined within the attributes of a JSX element.
The reasoning for 2 & 3 stems from previous discussions (for example #29883), that adding more checks in function parameters or jsx props would cause a lot of errors in existing JSX. This was also reflected in the tests for the previous iterations of this PR.