Skip to content

Slightly less conservative check in isConstraintPosition #46526

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Oct 26, 2021
Merged

Slightly less conservative check in isConstraintPosition #46526

merged 2 commits into from
Oct 26, 2021

Conversation

ahejlsberg
Copy link
Member

Fixes #46495.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Oct 25, 2021
@ahejlsberg
Copy link
Member Author

@typescript-bot test this
@typescript-bot user test this inline
@typescript-bot run dt
@typescript-bot perf test faster

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the inline community code test suite on this PR at 042debb. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the parallelized Definitely Typed test suite on this PR at 042debb. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the extended test suite on this PR at 042debb. You can monitor the build here.

@typescript-bot
Copy link
Collaborator

typescript-bot commented Oct 25, 2021 •
edited
Loading

Heya @ahejlsberg, I've started to run the abridged perf test suite on this PR at 042debb. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@ahejlsberg
The results of the perf run you requested are in!

Here they are:

Comparison Report - main..46526

Metric main 46526 Delta Best Worst
Angular - node (v14.15.1, x64)
Memory used 330,354k (± 0.01%) 330,340k (± 0.01%) -14k (- 0.00%) 330,285k 330,422k
Parse Time 1.95s (± 0.64%) 1.95s (± 0.59%) +0.01s (+ 0.36%) 1.94s 1.99s
Bind Time 0.86s (± 0.69%) 0.86s (± 0.72%) +0.00s (+ 0.35%) 0.85s 0.87s
Check Time 5.33s (± 0.43%) 5.33s (± 0.56%) +0.00s (+ 0.06%) 5.28s 5.41s
Emit Time 6.13s (± 0.50%) 6.11s (± 0.43%) -0.02s (- 0.28%) 6.05s 6.16s
Total Time 14.26s (± 0.26%) 14.25s (± 0.39%) -0.00s (- 0.04%) 14.15s 14.41s
Compiler-Unions - node (v14.15.1, x64)
Memory used 192,567k (± 0.49%) 193,213k (± 0.01%) +646k (+ 0.34%) 193,162k 193,262k
Parse Time 0.81s (± 0.69%) 0.81s (± 0.49%) +0.00s (+ 0.37%) 0.80s 0.82s
Bind Time 0.55s (± 0.40%) 0.56s (± 0.90%) +0.00s (+ 0.73%) 0.55s 0.57s
Check Time 7.53s (± 0.44%) 7.50s (± 0.22%) -0.03s (- 0.40%) 7.47s 7.56s
Emit Time 2.44s (± 0.71%) 2.44s (± 0.43%) +0.00s (+ 0.00%) 2.43s 2.48s
Total Time 11.34s (± 0.42%) 11.31s (± 0.16%) -0.02s (- 0.19%) 11.28s 11.38s
Monaco - node (v14.15.1, x64)
Memory used 324,010k (± 0.01%) 323,994k (± 0.01%) -16k (- 0.00%) 323,957k 324,031k
Parse Time 1.51s (± 0.62%) 1.50s (± 0.74%) -0.00s (- 0.27%) 1.48s 1.53s
Bind Time 0.75s (± 0.48%) 0.76s (± 0.63%) +0.00s (+ 0.40%) 0.75s 0.77s
Check Time 5.30s (± 0.42%) 5.29s (± 0.42%) -0.01s (- 0.25%) 5.22s 5.33s
Emit Time 3.20s (± 0.53%) 3.21s (± 0.68%) +0.01s (+ 0.31%) 3.18s 3.27s
Total Time 10.76s (± 0.31%) 10.76s (± 0.33%) -0.01s (- 0.06%) 10.70s 10.86s
TFS - node (v14.15.1, x64)
Memory used 288,365k (± 0.01%) 288,360k (± 0.01%) -4k (- 0.00%) 288,295k 288,403k
Parse Time 1.23s (± 0.70%) 1.24s (± 0.56%) +0.01s (+ 0.73%) 1.23s 1.26s
Bind Time 0.73s (± 0.76%) 0.74s (± 1.04%) +0.00s (+ 0.41%) 0.72s 0.76s
Check Time 4.90s (± 0.53%) 4.90s (± 0.31%) +0.01s (+ 0.14%) 4.87s 4.94s
Emit Time 3.48s (± 0.69%) 3.45s (± 0.72%) -0.03s (- 0.78%) 3.40s 3.49s
Total Time 10.34s (± 0.35%) 10.33s (± 0.26%) -0.01s (- 0.09%) 10.27s 10.39s
material-ui - node (v14.15.1, x64)
Memory used 447,290k (± 0.08%) 447,435k (± 0.03%) +146k (+ 0.03%) 446,836k 447,544k
Parse Time 1.82s (± 0.49%) 1.82s (± 0.37%) -0.00s (- 0.05%) 1.81s 1.84s
Bind Time 0.68s (± 0.54%) 0.68s (± 0.66%) +0.00s (+ 0.30%) 0.67s 0.69s
Check Time 13.09s (± 0.97%) 13.02s (± 0.50%) -0.07s (- 0.55%) 12.91s 13.23s
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) 0.00s ( NaN%) 0.00s 0.00s
Total Time 15.59s (± 0.84%) 15.53s (± 0.42%) -0.07s (- 0.43%) 15.41s 15.73s
xstate - node (v14.15.1, x64)
Memory used 533,836k (± 0.01%) 533,842k (± 0.00%) +6k (+ 0.00%) 533,779k 533,881k
Parse Time 2.55s (± 0.54%) 2.55s (± 0.40%) +0.00s (+ 0.08%) 2.53s 2.58s
Bind Time 1.15s (± 0.82%) 1.15s (± 1.41%) 0.00s ( 0.00%) 1.13s 1.21s
Check Time 1.55s (± 0.54%) 1.55s (± 0.61%) +0.01s (+ 0.32%) 1.53s 1.57s
Emit Time 0.07s (± 0.00%) 0.07s (± 0.00%) 0.00s ( 0.00%) 0.07s 0.07s
Total Time 5.32s (± 0.36%) 5.33s (± 0.26%) +0.01s (+ 0.13%) 5.31s 5.35s
System
Machine Namets-ci-ubuntu
Platformlinux 4.4.0-210-generic
Architecturex64
Available Memory16 GB
Available Memory10 GB
CPUs4 × Intel(R) Core(TM) i7-4770 CPU @ 3.40GHz
Hosts
  • node (v14.15.1, x64)
Scenarios
  • Angular - node (v14.15.1, x64)
  • Compiler-Unions - node (v14.15.1, x64)
  • Monaco - node (v14.15.1, x64)
  • TFS - node (v14.15.1, x64)
  • material-ui - node (v14.15.1, x64)
  • xstate - node (v14.15.1, x64)
Benchmark Name Iterations
Current 46526 10
Baseline main 10

Developer Information:

Download Benchmark

@typescript-bot
Copy link
Collaborator

@ahejlsberg
Great news! no new errors were found between main..refs/pull/46526/merge

@ahejlsberg
Copy link
Member Author

Tests look good and performance not affected. I think this one is good to go.

andrewbranch
andrewbranch reviewed Oct 26, 2021
View reviewed changes
tests/cases/conformance/controlFlow/controlFlowGenericTypes.ts
function update<T extends Control, K extends keyof T>(control : T | undefined, key: K, value: T[K]): void {
if (control !== undefined) {
control[key] = value;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can a subsequent else if discriminate between Button and Checkbox, or are we now stuck with T?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In a subsequent else if the type of control would be undefined. But if you mean a subsequent if inside the block, then yes:

if (control !== undefined) {          // Type of control is T | undefined
    control[key] = value;             // Type of control is T
    if (control.type === "button") {  // Type of control is Control
        control.text;                 // Type of control is Button
    }
}

@ahejlsberg ahejlsberg merged commit f424dfc into main Oct 26, 2021
@ahejlsberg ahejlsberg deleted the fix46495 branch October 26, 2021 18:54
mprobst pushed a commit to mprobst/TypeScript that referenced this pull request Jan 10, 2022
@ahejlsberg @mprobst
Slightly less conservative check in isConstraintPosition (microsoft#4…
94bad93 
…6526)

* Slight adjustment to check in isConstraintPosition

* Add regression test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@andrewbranch andrewbranch andrewbranch approved these changes

Assignees

@ahejlsberg ahejlsberg

Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Type 'keyof T' cannot be used to index type 'T extends <type>' with unions in v4.4.4 and newer
3 participants
@ahejlsberg @typescript-bot @andrewbranch