More type checking #373
Merged
More type checking #373
+1,451
−169
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jakebailey
reviewed
Feb 20, 2025
| props []string | ||
| } | ||
|
|
||
| var getFeatureMap = sync.OnceValue(func() map[string][]FeatureMapEntry { |
There was a problem hiding this comment.
This could probably just be a global; I don't think we need this to be lazy. Though I doubt the compiler is actually able to statically create this for us (so will in effect just create an init method to do it).
There was a problem hiding this comment.
I like having it be lazy. That way, in a program with no errors it will never be created.
There was a problem hiding this comment.
I feel like that's somewhat overkill given all of our other globals that will also never be used most of the time, but, it is what the old code did, so 😄
internal/checker/utilities.go
Outdated
| return scanner.GetRangeOfTokenAtPosition(ast.GetSourceFileOfNode(node), pos) | ||
| } | ||
|
|
||
| func symbolName(symbol *ast.Symbol) string { |
There was a problem hiding this comment.
I think this is duplicated with ast.SymbolName.
| @@ -456,15 +456,24 @@ var exclusivelyPrefixedNodeCoreModules = map[string]bool{ | |||
| } | |||
|
|
|||
| func (p *Program) GetEmitModuleFormatOfFile(sourceFile *ast.SourceFile) core.ModuleKind { | |||
| return p.GetEmitModuleFormatOfFileWorker(sourceFile, p.compilerOptions) | |||
There was a problem hiding this comment.
Do we need the workers here if we're just directly calling them?
There was a problem hiding this comment.
Probably not, I just ported what was there.
internal/core/core.go
Outdated
| minJ = 1 | ||
| } | ||
| maxJ := int(math.Floor(maxValue + float64(i))) | ||
| if maxJ > len(s2) { |
There was a problem hiding this comment.
When the merge conflicts go away and CI runs, I suspect that some of this will have lint messages asking to use max/min.
| @@ -131,6 +131,11 @@ CompilerOptions::{ | |||
| "tscBuild": null | |||
| } | |||
| Output:: | |||
| project/index.ts(1,19): error TS2307: Cannot find module '../utils' or its corresponding type declarations. | |||
There was a problem hiding this comment.
Hm, this is interesting; implies we aren't resolving something anymore?
There was a problem hiding this comment.
Actually, the issue is that we weren't resolving previously. I'm not sure what the purpose of this test is, but it looks like the error is legit.
jakebailey
approved these changes
Feb 20, 2025
|
Hm, this seems to have broken tests in #275, with traces like: Should really get those tests in now that they're fast. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
In this PR: