8000 track returned state in function body by vankop · Pull Request #14357 · webpack/webpack · GitHub
[go: up one dir, main page]
Skip to content

track returned state in function body #14357

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 22 commits into from
Feb 28, 2025
Merged

track returned state in function body #14357

merged 22 commits into from
Feb 28, 2025

Conversation

vankop
Copy link
Member
@vankop vankop commented Sep 28, 2021 •
edited
Loading

What kind of change does this PR introduce?

feature

closes #14347

Did you add tests for your changes?

yes

Does this PR introduce a breaking change?

no

What needs to be documented once your changes are merged?

nothing

Notes regarding feature design

Added 2 properties to parser current scope (parser.scope)

  • terminated marks that further parsing of nearest BlockStatement should be terminated. Has 3 states 'return'|'throw'|undefined, where undefined = false, 'return'|'throw' types of termination
    returned is inherited from child BlockStatement (or terminate node in case of if (true) return;) to nearest parent BlockStatement if executedPath=true
  • executedPath marks that current statement will be executed if nearest BlockStatement will be reached, e.g.
function a() {  // <- executedPath=true
  return; // <- terminated=return
  // should terminate parsing of current BlockStatement
}
function a() {  // <- executedPath=true
  if (rand()) { // <- executedPath=false marks 'unknown' state
     return 1; // terminated=return
  }  // <- terminated=undefined we do not know will this be executed or not (child block executedPath=false)
  
  if (true) {  // <- executedPath=true
       throw 2;  // <- terminated=throw
  }
   // should terminate parsing of current BlockStatement
   require('c')
}
function a() {  // <- executedPath=true
  for (;;) {  // <- executedPath=false for all loops
     require('a') // should be required
     if (true) {  // <- executedPath=true
         return;  // <- terminated=return
     }
     // should terminate parsing of current BlockStatement
     require('c')
  } // <- terminated=undefined
  
  try { // <- executedPath=true same executedPath for each inner block
     throw 2; // <- terminated=throw
     // should not be parsed
     require()
  } catch(e) {  // executedPath=false for every catch
   
  } // terminated=undefined throw in try
  // continue parsing..
}

complex example

function a() {  // <- executedPath=true
  if (rand()) {  // <- executedPath=false
    if (true) {  // <- executedPath=true
        return;  // <- terminated=return
    }
    // should terminate parsing of current BlockStatement
    require('c')
  }  // <- executedPath=true, terminated=undefined
  
  if (true) {  // <- executedPath=true
       throw 2;  // <- terminated=throw
  }  // <- executedPath=true, terminated=throw

   // should terminate parsing of current BlockStatement
   require('c')
}

and one more 🔞

function a(name) { // executedPath=true
switch (name) { // executedPath=false for each switch
  case "a": { // executedPath=false from parent
     if (true) return; // executedPath=true, terminated=return
     // skip parsing current block
     require('a')
     return 1;
  } // terminated=undefined since `case: a` block is executedPath=false
  case "b":
     if (true) return; // executedPath=true, terminated=return
     // skip parsing current switch case, but not current block ⚠️
     // we should not mark current block as returned=true
     require('a');
     return 2;
  case "c":
    // should be parsed
     return require('c');
  case "d":
    // // should be parsed
     return require('c');
  default:
     throw new Error("Unexcepted test data");
}
}

Notes regarding feature implementation

  • right now parser.hooks.terminate looks kind of useless, not sure about it..

TODO
drop skipped code. Any ideas how to do it better? we should do this from terminated!=undefined blocks till terminated=undefined blocks

@webpack-bot
Copy link
Contributor

For maintainers only:

  • This needs to be documented (issue in webpack/webpack.js.org will be filed when merged)
  • This needs to be backported to webpack 4 (issue will be created when merged)

vankop
vankop commented Oct 11, 2021
View reviewed changes
vankop
vankop commented Oct 11, 2021
View reviewed changes
@webpack-bot
Copy link
Contributor

Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon.

@sokra sokra self-assigned this Jan 31, 2022
@alexander-akait
Copy link
Member

@vankop Can you rebase? Thank you

@TheLarkInn
Copy link
Member
TheLarkInn commented Jun 5, 2023 •
edited by vankop
Loading
  • Add test case for hoisted function after return
  • Add test case for dynamic import after return
  • Change test case to use require("fail") instead of importing in existing module (so it actually fails compiling when parsing that part)

alexander-akait
alexander-akait reviewed Apr 11, 2024
View reviewed changes
lib/javascript/JavascriptParser.js
@@ -1799,6 +1805,7 @@ class JavascriptParser extends Parser {
for (let index = 0, len = statements.length; index < len; index++) {
const statement = statements[index];
this.walkStatement(statement);
if (this.scope.terminated) break;
Copy link
Member
8000

Choose a reason for hiding this comment

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

I like this solution, I'm only afraid of one thing, that some plugins could rely on it and rewrite something, and now we just will not handle them at all

Copy link
Member Author

Choose a reason for hiding this comment

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

but this code is unreachable.. so I dont see a reason to waste cpu =)

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree with you, I'm just worried that someone could use similar logic, but we can merge and look at feedback, It's not difficult to do a revert

@alexander-akait alexander-akait mentioned this pull request Jun 4, 2024
Closed
@alfaproject
Copy link

Is there a npm package version or tarball I can use to test this? We have a massively distributed platform and bundle everything so at worst something stops working and at best we save some bytes in said bundles (:

@alexander-akait
Copy link
Member

@alfaproject Are you about this Pr?

@alfaproject
Copy link

Yes, is there an easy way I can test these changes easily? If it's just waiting for feedback from testers, that is

@alexander-akait
Copy link
Member

@alfaproject You can install webpack from github, using package.json and run it

@alfaproject
Copy link

I can confirm that there were no issues with these changes in our code base

@alexander-akait alexander-akait mentioned this pull request Feb 14, 2025
Merged
@alexander-akait
Copy link
Member
alexander-akait commented Feb 14, 2025 •
edited
Loading

TODO:

@alexander-akait alexander-akait merged commit af7d788 into main Feb 28, 2025
52 of 63 checks passed
@alexander-akait alexander-akait deleted the feature-14347 branch February 28, 2025 14:39
@github-project-automation github-project-automation bot moved this from Ready for Merge to Need's Release in webpack 5/6 Feb 28, 2025
@hai-x hai-x mentioned this pull request May 15, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

Assignees

@sokra sokra

Labels
PR: CI-ok PR: reviewed-approved
Projects
webpack 5/6
Status: Need's Release
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Dead code eliminator for constant expression doesn't handle "return early" if statements
6 participants
@vankop @webpack-bot @alexander-akait @TheLarkInn @alfaproject @sokra
0