Check patterns for validity before putting them into pattern ambient modules #875
Conversation
internal/core/pattern.go
Outdated
| @@ -43,6 +43,9 @@ func FindBestPatternMatch[T any](values []T, getPattern func(v T) Pattern, candi | |||
| longestMatchPrefixLength := -1 | |||
| for _, value := range values { | |||
| pattern := getPattern(value) | |||
| if !pattern.IsValid() { | |||
There was a problem hiding this comment.
Consider adding a log statement or comment to explain why the invalid pattern is silently skipped to assist future debugging and ensure clarity of intent.
| if !pattern.IsValid() { | |
| if !pattern.IsValid() { | |
| log.Printf("Skipping invalid pattern: %+v", pattern) |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Is it just me or are these getting worse and worse
There was a problem hiding this comment.
It's not just you, they're awful now
There was a problem hiding this comment.
This is one way to do this which I'm not opposed to, but
- I would prefer nobody ever match against an invalid pattern to begin with, which leads to
- why is
declare globallanding in the list of patterns anyway? That should be fixed in the binder.
There was a problem hiding this comment.
I guess you mentioned
Global augmentations are stored as pattern ambient modules with zeroed patterns.
Was this always how it worked in Strada? Why is it done this way?
There was a problem hiding this comment.
I was also surprised and thought that might be a bug, but this is how it's designed in Strada. Not a bug.
There was a problem hiding this comment.
Correction: these conditions happen only under an error, where a declare global occurs in a script file. I misread the Strada binder code that handles this; it does omit it from the array. IOW you're right, my bad ❤️
internal/core/pattern.go
Outdated
| @@ -43,6 +43,9 @@ func FindBestPatternMatch[T any](values []T, getPattern func(v T) Pattern, candi | |||
| longestMatchPrefixLength := -1 | |||
| for _, value := range values { | |||
| pattern := getPattern(value) | |||
| if !pattern.IsValid() { | |||
There was a problem hiding this comment.
This is one way to do this which I'm not opposed to, but
- I would prefer nobody ever match against an invalid pattern to begin with, which leads to
- why is
declare globallanding in the list of patterns anyway? That should be fixed in the binder.
andrewbranch
changed the title
internal/binder/binder.go
Outdated
| @@ -806,7 +806,7 @@ func (b *Binder) bindModuleDeclaration(node *ast.Node) { | |||
| } | |||
| } | |||
| symbol := b.declareSymbolAndAddToSymbolTable(node, ast.SymbolFlagsValueModule, ast.SymbolFlagsValueModuleExcludes) | |||
| if pattern.StarIndex >= 0 { | |||
| if pattern.IsValid() && pattern.StarIndex >= 0 { | |||
There was a problem hiding this comment.
I'd prefer if we switched to something like
if pattern := core.TryParsePattern(...); pattern.IsValid() {
if pattern.StarIndex >= 0 {
b.file.PatternAmbientModules = append(b.file.PatternAmbientModules, &ast.PatternAmbientModule{Pattern: pattern, Symbol: symbol})
}
} else {
b.errorOnFirstToken(name, diagnostics.Pattern_0_can_have_at_most_one_Asterisk_character, name.AsStringLiteral().Text)
}There was a problem hiding this comment.
(I can't make the suggestion to above lines, I'm sorry)
There was a problem hiding this comment.
That code isn't correct, though. pattern can be invalid because it contained two stars or because it's the zero struct. The latter case occurs when we don't have a string literal to try to parse with. So that suggestion doesn't work both because we have no argument to pass to TryParsePattern and because the error issued is only valid for the double star case.
There was a problem hiding this comment.
I think my intent was to only do this work within if ast.IsStringLiteral(name). Sorry I didn't make that clearer.
There was a problem hiding this comment.
andrewbranch#1 is what I had in mind.
Only create and track possible patterns when dealing with string literals.
Fixes #870
Global augmentations are stored as pattern ambient modules with zeroed patterns.Invalid global augmentations were falling through binder cases and getting put into pattern ambient modules, when they should have been skipped.