Skip to content
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

Prevent errors when using enums in a file that's not included in any scopes #995

Conversation

josephjunker
Copy link
Collaborator

When running compilation on one of our projects I encountered this error:

Error when calling plugin BscPlugin.beforeFileTranspile: TypeError: Cannot read properties of undefined (reading 'getEnumMemberFileLink')
    at BrsFilePreTranspileProcessor.getEnumInfo (/Users/jjunker/code/my-application/common/temp/node_modules/.pnpm/brighterscript@0.65.14/node_modules/brighterscript/dist/bscPlugin/transpile/BrsFilePreTranspileProcessor.js:38:32)
    <long stack trace here>

This occurs when a file which uses enums is not found in any scope. The relevant unit test wasn't testing enums and was suppressing errors, so I expanded the functionality of the unit test. The actual fix is the added null coalescing operator on line 44 of src/bscPlugin/transpile/BrsFilePreTranspileProcessor.ts.

There are some changes here that are not directly tied to the bug fix at hand. This is because I traced through other usages of Program.getFirstScopeForFile to check whether any other locations might have the same error. Doing so lead me to src/bscPlugin/CallExpressionInfo.ts. This file correctly checks for nullishness and so contains no errors, but the type signatures weren't fully specific and I changed them while debugging. I can back out these changes if so desired.

I also added | undefined to a few signatures for cases where a value may be undefined. I find this to be helpful even with strict null checks turned off, but I also can tell that it's unidiomatic for this codebase so I can remove these additional annotations if you'd prefer the change without them.

Copy link
Member

@TwitchBronBron TwitchBronBron left a comment

Choose a reason for hiding this comment

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

Looks pretty good! One small suggestion, but otherwise I'm happy with all of these changes. Thanks for the great explanation.

@@ -3,6 +3,8 @@ import * as fsExtra from 'fs-extra';
import { Program } from '../../Program';
import { standardizePath as s } from '../../util';
import { tempDir, rootDir } from '../../testHelpers.spec';
import { Logger, LogLevel } from '../..';
Copy link
Member

Choose a reason for hiding this comment

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

Can you change this to point to the actual logger file? Sometimes importing ../.. causes circular references.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fixed!

@TwitchBronBron TwitchBronBron merged commit 8be28cd into rokucommunity:master Dec 24, 2023
6 checks passed
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.

None yet

2 participants