Skip to content

fix: improve panic message for unspecified ScriptKind in Parser::initializeState #1556

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

camc314
Copy link
Contributor

@camc314 camc314 commented Aug 11, 2025

Currently this message give no diagnostic information, and no information about how to fix the issue.

ScriptKindUnknown, probably comes from here when the extension failed to be fetched, by including the filename in the diagnostic, it should allow devs/users to more easily track down the source of the panic

func GetScriptKindFromFileName(fileName string) ScriptKind {
dotPos := strings.LastIndex(fileName, ".")
if dotPos >= 0 {
switch strings.ToLower(fileName[dotPos:]) {
case tspath.ExtensionJs, tspath.ExtensionCjs, tspath.ExtensionMjs:
return ScriptKindJS
case tspath.ExtensionJsx:
return ScriptKindJSX
case tspath.ExtensionTs, tspath.ExtensionCts, tspath.ExtensionMts:
return ScriptKindTS
case tspath.ExtensionTsx:
return ScriptKindTSX
case tspath.ExtensionJson:
return ScriptKindJSON
}
}
return ScriptKindUnknown
}

Ref: oxc-project/oxc#12950

@jakebailey
Copy link
Member

I think we avoid including user-specified data in panics/throws just because that means we can't collect it for telemetry on errors. But maybe the old code already broke this rule?

@camc314
Copy link
Contributor Author

camc314 commented Aug 11, 2025

Hmm that's a fair point, feel free to close. However i do feel that the value in extra debugging info is valuable.

A large number of the panics do have additional data:

Total panics: (ignoring _test.go files)

$ rg -t go --count-matches --no-filename '\bpanic\s*\('  -g '!**/*_test.go' | awk '{s+=$1} END{print s}'
538

Panics with either string concatenation, or string formatting:

$ rg -t go --count-matches --no-filename -g '!**/*_test.go'   -e 'panic\(\s*fmt\.\w+\s*\('   -e 'panic\(\s*(?:"(?:[^"\\]|\\.)*"|`[^`]*`)\s*\+' | awk '{s+=$1} END{print s}'
161

For telemetry, couldn't just the first part of the string be matched?

@jakebailey
Copy link
Member

Searching for just prints in panics isn't really enough, given many of those aren't printing out filenames or file contents.

We'll have to think about this, but definitely be aware that we don't even have an API at the moment, so oxc's usage really comes without warranty (they're probably "holding it wrong", since we haven't exactly optimized for anyone using it outside our own). 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants