-
Notifications
You must be signed in to change notification settings - Fork 89
Fix race condition with setExperimentalFeatures #8637
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,33 @@ | ||
| import { recast } from '@src/lang/wasm' | ||
| import { err } from '@src/lib/trap' | ||
| import { join } from 'path' | ||
| import { loadAndInitialiseWasmInstance } from '@src/lang/wasmUtilsNode' | ||
| import { setExperimentalFeatures } from '@src/lang/modifyAst/settings' | ||
| const WASM_PATH = join(process.cwd(), 'public/kcl_wasm_lib_bg.wasm') | ||
|
|
||
| describe('settings.spec.ts', () => { | ||
| describe('Testing setExperimentalFeatures', () => { | ||
| it('should add the annotation and set the flag if not present', async () => { | ||
| const instance = await loadAndInitialiseWasmInstance(WASM_PATH) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is perfection |
||
| const newAst = setExperimentalFeatures('', { type: 'Allow' }, instance) | ||
| if (err(newAst)) { | ||
| throw newAst | ||
| } | ||
|
|
||
| const newCode = recast(newAst, instance) | ||
| expect(newCode).toBe(`@settings(experimentalFeatures = allow)\n`) | ||
| }) | ||
|
|
||
| it('should set the flag if the annotation exists', async () => { | ||
| const instance = await loadAndInitialiseWasmInstance(WASM_PATH) | ||
| const code = `@settings(experimentalFeatures = warn)\n` | ||
| const newAst = setExperimentalFeatures(code, { type: 'Deny' }, instance) | ||
| if (err(newAst)) { | ||
| throw newAst | ||
| } | ||
|
|
||
| const newCode = recast(newAst, instance) | ||
| expect(newCode).toBe(`@settings(experimentalFeatures = deny)\n`) | ||
| }) | ||
| }) | ||
| }) | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,27 @@ | ||
| import type { Node } from '@rust/kcl-lib/bindings/Node' | ||
| import type { WarningLevel } from '@rust/kcl-lib/bindings/WarningLevel' | ||
| import { err } from '@src/lib/trap' | ||
| import { changeExperimentalFeatures, parse, type Program } from '@src/lang/wasm' | ||
| import type { ModuleType } from '@src/lib/wasm_lib_wrapper' | ||
|
|
||
| export function setExperimentalFeatures( | ||
| code: string, | ||
| level: WarningLevel, | ||
| instance?: ModuleType | ||
| ): Node<Program> | Error { | ||
| const newCode = changeExperimentalFeatures(code, level, instance) | ||
| if (err(newCode)) { | ||
| return newCode | ||
| } | ||
|
|
||
| const result = parse(newCode, instance) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't love how we need a wrapper function on the TS side to parse the result of setting the As it is isn't fundamentally wrong, though. |
||
| if (err(result)) { | ||
| return result | ||
| } | ||
|
|
||
| if (result.program === null) { | ||
| return new Error('Empty program returned') | ||
| } | ||
|
|
||
| return result.program | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -816,10 +816,14 @@ export function changeDefaultUnits( | |
| */ | ||
| export function changeExperimentalFeatures( | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not changing anything here, but making it possible to run in vitest |
||
| kcl: string, | ||
| warningLevel: WarningLevel | null = null | ||
| warningLevel: WarningLevel | null = null, | ||
| instance?: ModuleType | ||
| ): string | Error { | ||
| try { | ||
| return change_experimental_features(kcl, JSON.stringify(warningLevel)) | ||
| const level = JSON.stringify(warningLevel) | ||
| return instance | ||
| ? instance.change_experimental_features(kcl, level) | ||
| : change_experimental_features(kcl, level) | ||
| } catch (e) { | ||
| console.error('Caught error changing kcl settings', e) | ||
| return new Error('Caught error changing kcl settings', { cause: e }) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,15 +1,13 @@ | ||
| import type { WarningLevel } from '@rust/kcl-lib/bindings/WarningLevel' | ||
| import { executeAstMock } from '@src/lang/langHelpers' | ||
| import { | ||
| type SourceRange, | ||
| type KclValue, | ||
| formatNumberValue, | ||
| parse, | ||
| resultIsOk, | ||
| changeExperimentalFeatures, | ||
| } from '@src/lang/wasm' | ||
| import type { KclExpression } from '@src/lib/commandTypes' | ||
| import { codeManager, kclManager, rustContext } from '@src/lib/singletons' | ||
| import { rustContext } from '@src/lib/singletons' | ||
| import { err } from '@src/lib/trap' | ||
| import type { ModuleType } from '@src/lib/wasm_lib_wrapper' | ||
| import type RustContext from '@src/lib/rustContext' | ||
|
|
@@ -153,15 +151,3 @@ export async function stringToKclExpression( | |
| export function getStringValue(code: string, range: SourceRange): string { | ||
| return code.slice(range[0], range[1]).replaceAll(`'`, ``).replaceAll(`"`, ``) | ||
| } | ||
|
|
||
| export async function setExperimentalFeatures( | ||
| level: WarningLevel | ||
| ): Promise<void | Error> { | ||
| const newCode = changeExperimentalFeatures(codeManager.code, level) | ||
| if (err(newCode)) { | ||
| return new Error(`Failed to set experimental features: ${newCode.message}`) | ||
| } | ||
| codeManager.updateCodeStateEditor(newCode) | ||
| await codeManager.writeToFile() | ||
| await kclManager.executeCode() | ||
| } | ||
|
Comment on lines
-156
to
-167
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved to |
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how I discovered this in #8565