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

Move pure comment annotation to Graph.contextParse #3981

Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 32 additions & 6 deletions src/Graph.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ import { BuildPhase } from './utils/buildPhase';
import { errImplicitDependantIsNotIncluded, error } from './utils/error';
import { analyseModuleExecution } from './utils/executionOrder';
import { PluginDriver } from './utils/PluginDriver';
import { markPureCallExpressions } from './utils/pureComments';
import relativeId from './utils/relativeId';
import { timeEnd, timeStart } from './utils/timers';
import { markModuleAndImpureDependenciesAsExecuted } from './utils/traverseStaticDependencies';
Expand Down Expand Up @@ -45,7 +46,6 @@ function normalizeEntryModules(
export default class Graph {
acornParser: typeof acorn.Parser;
cachedModules: Map<string, ModuleJSON>;
contextParse: (code: string, acornOptions?: acorn.Options) => acorn.Node;
deoptimizationTracker: PathTracker;
entryModules: Module[] = [];
moduleLoader: ModuleLoader;
Expand Down Expand Up @@ -77,11 +77,6 @@ export default class Graph {
for (const key of Object.keys(cache)) cache[key][0]++;
}
}
this.contextParse = (code: string, options: Partial<acorn.Options> = {}) =>
this.acornParser.parse(code, {
...(this.options.acorn as acorn.Options),
...options
});

if (watcher) {
this.watchMode = true;
Expand Down Expand Up @@ -117,6 +112,37 @@ export default class Graph {
this.phase = BuildPhase.GENERATE;
}

contextParse(code: string, options: Partial<acorn.Options> = {}) {
const onCommentOrig = options.onComment;
const comments: acorn.Comment[] = [];

if (onCommentOrig && typeof onCommentOrig == 'function') {
options.onComment = (block, text, start, end, ...args) => {
comments.push({type: block ? "Block" : "Line", value: text, start, end});
return onCommentOrig.call(options, block, text, start, end, ...args);
}
} else {
options.onComment = comments;
}

const ast = this.acornParser.parse(code, {
...(this.options.acorn as acorn.Options),
...options
});

if (typeof onCommentOrig == 'object') {
onCommentOrig.push(...comments);
}

options.onComment = onCommentOrig;

markPureCallExpressions(comments.map((cmt) => {
return {...cmt, block: cmt.type == "Block", text: cmt.value};
Copy link
Member

Choose a reason for hiding this comment

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

This mapping strikes me as unnecessary if you either push the comments into the array in the previous way or change markPureCallExpressions to work with a slightly changed format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree. I will change it.

In general I try to keep my changes minimal because it's my first contribution and I don't want to accidentally break anything...

}), ast);

return ast;
}

getCache(): RollupCache {
// handle plugin cache eviction
for (const name in this.pluginCache) {
Expand Down
59 changes: 27 additions & 32 deletions src/Module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,6 @@ import { getOrCreate } from './utils/getOrCreate';
import { getOriginalLocation } from './utils/getOriginalLocation';
import { makeLegal } from './utils/identifierHelpers';
import { basename, extname } from './utils/path';
import { markPureCallExpressions } from './utils/pureComments';
import relativeId from './utils/relativeId';
import { RenderOptions } from './utils/renderHelpers';
import { SOURCEMAPPING_URL_RE } from './utils/sourceMappingURL';
Expand Down Expand Up @@ -122,35 +121,6 @@ export interface AstContext {
warn: (warning: RollupWarning, pos: number) => void;
}

function tryParse(
module: Module,
Parser: typeof acorn.Parser,
acornOptions: acorn.Options
): acorn.Node {
try {
return Parser.parse(module.info.code!, {
...acornOptions,
onComment: (block: boolean, text: string, start: number, end: number) =>
module.comments.push({ block, text, start, end })
});
} catch (err) {
let message = err.message.replace(/ \(\d+:\d+\)$/, '');
if (module.id.endsWith('.json')) {
message += ' (Note that you need @rollup/plugin-json to import JSON files)';
} else if (!module.id.endsWith('.js')) {
message += ' (Note that you need plugins to import files that are not JavaScript)';
}
return module.error(
{
code: 'PARSE_ERROR',
message,
parserError: err
},
err.pos
);
}
}

const MISSING_EXPORT_SHIM_DESCRIPTION: ExportDescription = {
identifier: null,
localName: MISSING_EXPORT_SHIM_VARIABLE
Expand Down Expand Up @@ -719,13 +689,12 @@ export default class Module {

this.alwaysRemovedCode = alwaysRemovedCode || [];
if (!ast) {
ast = tryParse(this, this.graph.acornParser, this.options.acorn as acorn.Options);
ast = this.tryParse();
for (const comment of this.comments) {
if (!comment.block && SOURCEMAPPING_URL_RE.test(comment.text)) {
this.alwaysRemovedCode.push([comment.start, comment.end]);
}
}
markPureCallExpressions(this.comments, ast);
}

timeEnd('generate ast', 3);
Expand Down Expand Up @@ -834,6 +803,32 @@ export default class Module {
return null;
}

tryParse(): acorn.Node {
try {
return this.graph.contextParse(this.info.code!, {
onComment: (block: boolean, text: string, start: number, end: number) =>
this.comments.push({ block, text, start, end })
});
} catch (err) {
let message = err.message.replace(/ \(\d+:\d+\)$/, '');
if (this.id.endsWith('.json')) {
message += ' (Note that you need @rollup/plugin-json to import JSON files)';
} else if (!this.id.endsWith('.js')) {
message += ' (Note that you need plugins to import files that are not JavaScript)';
}
console.log(err);
return this.error(
{
code: 'PARSE_ERROR',
message,
parserError: err
},
err.pos
);
}
}


updateOptions({
meta,
moduleSideEffects,
Expand Down
2 changes: 1 addition & 1 deletion src/utils/PluginContext.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ export function getPluginContexts(
const moduleIds = graph.modulesById.keys();
return wrappedModuleIds();
},
parse: graph.contextParse,
parse: graph.contextParse.bind(graph),
resolve(source, importer, { custom, skipSelf } = BLANK) {
return graph.moduleLoader.resolveId(source, importer, custom, skipSelf ? pidx : null);
},
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
const assert = require('assert');

module.exports = {
description: 'external function calls marked with pure comment do not have effects and should be removed even if parsed by PluginContext.parse method',
options: {
external: ['socks'],
plugins:[{
transform(code, _id) {
const comments = [];
const ast = this.parse(code, {onComment: comments});
if (comments.length != 5 || comments.some(({value}) => !value.includes('PURE'))) {
throw new Error('failed to get comments');
}
return {ast, code, map: null};
},
}],
},
context: {
require(id) {
if (id === 'socks') {
return () => {
throw new Error('Not all socks were removed.');
};
}
}
},
code(code) {
assert.ok(code.search(/socks\(\)/) === -1);
}
};
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
import socks from 'socks';

/*#__PURE__*/ socks();
/* #__PURE__*/ socks();
/*#__PURE__ */ socks();
/* #__PURE__ */ socks();
// #__PURE__
socks();