Apply (Un)Capitalize template string mappings on generic template literal types in a safe manner #52112
Conversation
|
The problem is not with the mapping being applied to the generic template literals but with the mapping logic of According to So Edit: Or actually even // pass the type instead of texts and types
const { texts, types } = type
// ...
case IntrinsicTypeKind.Capitalize:
return (
texts[0] === "" ? type :
getTemplateLiteralType([texts[0].charAt(0).toUpperCase() + texts[0].slice(1), ...texts.slice(1)], types)
)
case IntrinsicTypeKind.Uncapitalize:
// similar change |
|
Ye, I also thought about applying the fix in those mapping functions - I just came to the conclusion that this fix is potentially easier and that it automatically makes it safer to introduce new mappings as this is an edge case that is easy to forget. I'm happy to adjust the fix if requested though. |
|
But regardless of whether we simplify generic template literals or not, the mapping logic of |
|
When that would manifest itself outside of generic contexts though? |
|
If there's a logical error in the code it should be fixed whether or not the control will reach that branch. Come on Mateusz, I'm pretty sure you know all this ;P But to answer your question, yes today a mapping would never apply to a non-generic template literal, because all non-generic template literals immediately simplify to string literals. But "today" is key here, it's possible this is no longer true in future. Eg in future it's possible that we support things like this... type Digit = "0" | "1" | "2" | ... | "9"
type Digits = `${Digit}${"" | Digits}`Here the template literal is not generic as it has no non-generic types and yet it can't be simplified to a string literal. So those mappings can be applied to such non-generic template literals which can lead to a bug because of the incorrect logic in them. I can give you an even more simple example though: The simplification of non-generic template literals into string literals that happens today is not a lot more than an optimization, it can be pulled anytime in future, ie Again all this is just to answer your question, this is not the reason to fix the logical error, the reason to fix the logical error is that it's a logical error. |
… safe way for (Un)Capitalize Co-authored-by: Devansh Jethmalani <[email protected]>
|
IMHO, if we can't write a test case that validates a change today - then we shouldn't introduce a change. We can't predict the future and it makes sense to write the code with the current invariants of the behavior in mind. Code grows and evolves over time - I often remove parts of the code and rerun tests to learn why the given code is needed and how the system works, so the ideal situation, for me, is that some tests fail after removing any given line/condition/whatever. If the tests don't fail - that is a strong signal to me that the code is redundant and that it can be removed. That being said - I came to the conclusion that I like the idea of reducing/simplifying as much as possible~ upfront. I've applied your suggestion, thanks! |
Andarist
changed the title
There's a bit of a confusion here, we're comparing my change against yours, not my change on top of yours against yours. My change is very much testable, the linked issue being a failing test which would then pass with the change.
Aside the fact I don't fully agree with this and without going into why, this is not in contradiction with what I said because I never said the fix is for the future or something, I said...
I think we've deviated way too much, the reason for the linked bug was the logical error in
No problems! |
|
@typescript-bot test this |
|
Heya @jakebailey, I've started to run the extended test suite on this PR at 27c593f. You can monitor the build here. |
|
Heya @jakebailey, I've started to run the diff-based user code test suite on this PR at 27c593f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the abridged perf test suite on this PR at 27c593f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 27c593f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the diff-based top-repos suite on this PR at 27c593f. You can monitor the build here. Update: The results are in! |
|
Heya @jakebailey, I've started to run the tarball bundle task on this PR at 27c593f. You can monitor the build here. |
|
Hey @jakebailey, I've packed this into an installable tgz. You can install it for testing by referencing it in your and then running There is also a playground for this build and an npm module you can use via |
|
@jakebailey Here are the results of running the user test suite comparing There were infrastructure failures potentially unrelated to your change:
Otherwise... Something interesting changed - please have a look. Details
|
|
@jakebailey Here they are:Comparison Report - main..52112
System
Hosts
Scenarios
Developer Information: |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@jakebailey Here are the results of running the top-repos suite comparing Everything looks good! |
|
Hey @jakebailey, it looks like the DT test run failed. Please check the log for more details. |
|
@typescript-bot run dt |
|
Heya @jakebailey, I've started to run the parallelized Definitely Typed test suite on this PR at 27c593f. You can monitor the build here. Update: The results are in! |
|
Hey @jakebailey, the results of running the DT tests are ready. |
fixes #52102