fix: check whether RegExp have the global or sticky flags set

[TrickyPi]

This PR contains:

• bugfix
• feature
• refactor
• documentation
• other

Are tests included?

• yes (bugfixes and features will not be merged without tests)
• no

Breaking Changes?

• yes (breaking changes will not be merged unless absolutely necessary)
• no

List any relevant issue numbers:

relative #4737

Description

[codecov]

Codecov Report

Merging #4742 (dea26bc) into master (243eba5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #4742   +/-   ##
=======================================
  Coverage   99.03%   99.04%           
=======================================
  Files         217      217           
  Lines        7706     7709    +3     
  Branches     2132     2134    +2     
=======================================
+ Hits         7632     7635    +3     
  Misses         24       24           
  Partials       50       50           

Impacted Files	Coverage Δ
src/ast/values.ts	100.00% <ø> (ø)
src/ast/nodes/Literal.ts	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[lukastaegert]

I see, so the problem is that for a sticky/global regular expression, we cannot drop usages because they have internal state that changes. Thank you for picking up on this quickly!
In that case, it would actually be nicer to have a test in the function folder that contains these regular expressions and would error if one of them was removed.

I.e. for global it might look something like this:

var globalRegExp = /1/g;
globalRegExp.exec('1'); // must not be removed
assert.strictEqual(globalRegExp.exec('1'), null);

and similar for the sticky flag. I think it would be still ok to put both flags into the same test for code organization. That would also document WHY we want to keep them in case anyone messes with the logic in the future, and ensures they do not break valid use cases.

However for this example, we can even do better than just never removing them. As you can see, we only need to keep the regular expressions if we actually use them—otherwise, the side effect does not matter. This is very similar e.g. to what we do for arrays.

Now a simple way to pull this off would be to check if the regular expression is "included", which is a flag any AST node gets once it is needed for some reason in the bundle. In this case, it would be sufficient indication that the regular expression value is actually used.

This could be done by extending hasEffectsOnInteractionAtPath:

switch (interaction.type) {
	// ...
	case INTERACTION_CALLED: {
		if (
			this.value instanceof RegExp &&
			this.included &&
			(this.value.global || this.value.sticky)
		) {
			return true;
		}
		return (
			path.length !== 1 ||
			hasMemberEffectWhenCalled(this.members, path[0], interaction, context)
		);
	}
}

I think that would be good enough. Alternatively if you do not want to separate the definition of the methods from the special logic for global/sticky, one could also extend hasMemberEffectWhenCalled with a fifth parameter that contains the AST node itself.

Then we could do it similar to how we handle side effects in the callback argument of string.replace

This is done via a custom call-effect handler

rollup/src/ast/values.ts

Lines 156 to 174 in cf06c30

	const stringReplace: RawMemberDescription = {
	value: {
	hasEffectsWhenCalled({ args }, context) {
	const argument1 = args[1];
	return (
	args.length < 2 ||
	(typeof argument1.getLiteralValueAtPath(EMPTY_PATH, SHARED_RECURSION_TRACKER, {
	deoptimizeCache() {}
	}) === 'symbol' &&
	argument1.hasEffectsOnInteractionAtPath(
	EMPTY_PATH,
	NODE_INTERACTION_UNKNOWN_CALL,
	context
	))
	);
	},
	returns: UNKNOWN_LITERAL_STRING
	}
	};

You would also need to add the AST node as a parameter to the call-effect handler, then you could move the logic to these methods and e.g. .toString() would always be side effect free, even on a stateful global regular expression.

[TrickyPi]

Thank you so much for your great guidance. I pushed a commit(efb6544) just now. In this commit, i added some tests in the form folder, because i think we need to check the result of the common and used RegExp, e.g. /1/, and I wonder if these tests could be merged into here test/form/samples/builtin-prototypes/literal/main.js.

As for the solution, I tried the second, but I found that a lot of code needs to be changed in the process, and there are some codes I don't understand. So I choose the first.

[rollup-bot]

This PR has been released as part of rollup@3.7.4. You can test it via npm install rollup.