source-academy / js-slang

Implementations of sublanguages of JavaScript, TypeScript, Scheme and Python
https://source-academy.github.io/source/
Apache License 2.0
70 stars 104 forks source link

Import Analyzer and Augmentations #1507

Closed leeyi45 closed 7 months ago

leeyi45 commented 1 year ago

All import declarations are checked for undefined imports, including local modules, so importing a symbol that isn't exported by a module will throw an error. Furthermore, the preprocessor and transpiler have been augmented to be able handle all 3 types of import specifiers. Though namespace imports may not be allowed in Source, the functionality has been incorporated so that the full JS and TS runners may make use of them.

Currently js-slang assumes that multiple files means that the source file path should be included with the AST. I've provided an option under IOptions to add the file path information to the AST. This is so that even without knowing beforehand the number of files required, file path information can be added.

As per @ianyong's suggestions, I've kept the number of type assertions and scale of these changes to a minimum.

I'm not 100% sold on some of the names I've given things (e.g. linker.ts), so any suggestions regarding those would be appreciated.

coveralls commented 11 months ago

Pull Request Test Coverage Report for Build 8409238417

Details


Changes Missing Coverage Covered Lines Changed/Added Lines %
src/runner/sourceRunner.ts 8 9 88.89%
src/transpiler/transpiler.ts 12 13 92.31%
src/modules/preprocessor/analyzer.ts 70 72 97.22%
src/modules/preprocessor/index.ts 56 58 96.55%
src/cse-machine/interpreter.ts 9 13 69.23%
src/interpreter/interpreter.ts 11 15 73.33%
src/utils/dict.ts 22 30 73.33%
src/modules/errors.ts 43 52 82.69%
src/name-extractor/index.ts 36 45 80.0%
src/stepper/stepper.ts 4 14 28.57%
<!-- Total: 548 598 91.64% -->
Files with Coverage Reduction New Missed Lines %
src/runner/sourceRunner.ts 1 84.34%
src/parser/scheme/index.ts 1 94.12%
src/stepper/stepper.ts 1 65.6%
src/cse-machine/interpreter.ts 1 91.86%
src/name-extractor/index.ts 3 70.88%
src/schedulers.ts 11 72.5%
<!-- Total: 18 -->
Totals Coverage Status
Change from base Build 8408541784: 0.2%
Covered Lines: 10585
Relevant Lines: 12636

💛 - Coveralls
leeyi45 commented 9 months ago

I've noticed some serious bugs not being caught by tests, let me work on those before reopening this

leeyi45 commented 8 months ago

This really should be merged soon....

leeyi45 commented 8 months ago

There's not really a main motivation: I just thought that since namespace imports are supported in fullJS I might as well implement it along the way

leeyi45 commented 7 months ago

Great work overall, just a few nits.

Could you guide me through the execution flow which enables this test case:

        '/a.js': `import * as a from 'one_module';`,
        '/b.js': `import * as a from 'one_module';`

to throw DuplicateImportNameError? It seems to me that although namespaceSpecifiers.length > 0, regularSpecifiers.length is 0, therefore this predicate

        inamespaceSpecifiers.length > 0 && regularSpecifiers.length > 0) ||
        importedNames.size > 1

will be false? I think I'm missing something here

This case shouldn't trigger a DuplicateImportError. Is there a case in the tests that indicates that this should throw an error?