-
Notifications
You must be signed in to change notification settings - Fork 104
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
Further Enhancements to the Module System #1428
Conversation
Pull Request Test Coverage Report for Build 5406352152
💛 - Coveralls |
…ports-clean # Conflicts: # src/ec-evaluator/interpreter.ts # src/interpreter/closure.ts
…ports-clean # Conflicts: # sicp_publish/yarn.lock # src/ec-evaluator/interpreter.ts # src/index.ts # src/runner/sourceRunner.ts # src/stepper/lib.ts # yarn.lock
I have since taken several of @ianyong's suggestions into account and modified this PR appropriately, the last remaining issue is to solve the error that comes up when running |
As a comment for the EC-Evaluator team @dickongwd @vansh284, would there be an issue if the Program command evaluator were written as part of the function ECEvaluate(...) {
for (const node of program.body) {
// Evaluate imports first
}
for (const node of program.body) {
// Run command evaluators
}
} This makes it easier to pass options to the the code that handles module imports, and also removes the need to make every evaluator async. |
You are right in saying that Programs wouldn't contain other programs as children, so the final evaluation would still be the same. However, (I'm not sure if you are aware) the 'agenda' in the ec-evaluator is visualised in the frontend as part of the Env Visualizer. The command evaluators act on each of the 'agenda' items, so if we have no command evaluator for the Program, it cannot be pushed on the 'agenda'. So if instead of having a Program command evaluator, we directly evaluate the program at the start, the students wouldn't be able to see the first step of the visualization (which is the Program item on the agenda) as it would be skipped. Maybe you can let us know exactly what your goal is, so we can think of a way to make it work with the ec-evaluator. |
Perhaps I could refer to the implementation within the stepper: async function evaluateImports(
program: es.Program,
context: Context,
{ loadTabs, wrapModules }: ImportTransformOptions
) {
let importNodes: es.ImportDeclaration[]
;[importNodes, program.body] = partition(program.body, importDeclarationFilter)
try {
const importNodeMap = arrayMapFrom(
importNodes.map(node => {
const moduleName = node.source.value
assert(
typeof moduleName === 'string',
`ImportDeclarations should have string sources, got ${moduleName}`
)
return [moduleName, node]
})
)
const environment = currentEnvironment(context)
await importNodeMap.forEachAsync(async (moduleName, nodes) => {
await initModuleContextAsync(moduleName, context, loadTabs, nodes[0])
const functions = await loadModuleBundleAsync(moduleName, context, wrapModules, nodes[0])
for (const node of nodes) {
for (const spec of node.specifiers) {
let importedName: string
switch (spec.type) {
case 'ImportSpecifier': {
importedName = spec.imported.name
break
}
case 'ImportDefaultSpecifier': {
importedName = 'default'
break
}
case 'ImportNamespaceSpecifier': {
throw new Error('Namespace imports are not supported!')
}
}
declareIdentifier(context, spec.local.name, node, environment)
defineVariable(context, spec.local.name, functions[importedName], true, node)
}
}
})
} catch (error) {
// console.log(error)
handleRuntimeError(context, error)
}
} This is directly called by // the context here is for builtins
export async function getEvaluationSteps(
program: es.Program,
context: Context,
stepLimit: number | undefined,
importOptions: ImportTransformOptions
): Promise<[es.Program, string[][], string][]> {
const steps: [es.Program, string[][], string][] = []
try {
const limit = stepLimit === undefined ? 1000 : stepLimit % 2 === 0 ? stepLimit : stepLimit + 1
await evaluateImports(program, context, importOptions)
// starts with substituting predefined constants
// etc... I am able to access the |
Due to the circumstances, I was unable to work on this. Given the large number of changes that have been merged since I will reopen this with a fresh branch |
Notable Changes
export from
resolutionWhen using
export from
declarations, the system will now resolve the symbol through the directives to where the symbol was defined:This change may cause snapshots that use import directives to produce different import statements when preprocessed.
Should fix #1416 by introducing support for such export declarations
Asynchronous Imports
Addresses #1348 partially by introducing a new set of functions that use
fetch
to retrieve module files. However, the interpreter does not support asynchronous operation, so the original synchronous module loading code has been left in.Import Preprocessor
The import preprocessor has been changed to treat local and source imports more uniformly:
export *
statements are not currently supported by other language features, the preprocessor has the ability to detect reexported symbols should such statements become available in the futureimport a from './a'
to be resolved toimport a from './a/index.js'
, similar to how Node and browsers resolve modules. This should help work toward the js-slang repl being able to execute multiple filesAssertions
Introduce a new
AssertionError
type that derives fromRuntimeSourceError
to gracefully handle unexpected conditions that arise during program execution, along side anassert
function. Using a custom function allows us to modify the errors assertions throw, and removes the need for polyfills for the NodeJS assert module to work with the browser.Minor Changes
Execution Options
logTranspilerOutput
have been addedRefactoring of AST utils
src/utils/ast
. Where possible, type guards that were spread out across different parts of js-slang have been consolidated.import * as es from './utils/ast/types'
can be used to replaceimport * as es from 'estree'
. This will provide extra AST node types and reduce the amount of type checking needed when dealing with an AST. All types are compatible withestree
.