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

Remove unused catch binding #6048

Merged
merged 15 commits into from Sep 19, 2017
Merged

Remove unused catch binding #6048

merged 15 commits into from Sep 19, 2017

Conversation

@MarckK
Copy link
Member

MarckK commented Aug 3, 2017

Q A
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature? yes
Deprecations?
Spec Compliancy?
Tests Added/Pass?
Fixed Tickets
License MIT
Doc PR
Dependency Changes

codemod to remove unused catch binding

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Aug 3, 2017

Awesome (regarding where this goes, making the PR wherever is fine), I was almost thinking maybe each proposal should include the codemod version of itself too, but then I guess it would be hard to use since you need a cli... So maybe it would be better (later) as part of another tool like lebab/babel-update.

@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch from c6dba28 to 7663e57 Aug 4, 2017
@MarckK MarckK changed the title [WIP] Remove unused catch binding Remove unused catch binding Aug 4, 2017
!path.scope.getOwnBinding(path.node.param.name).referenced
) {
const paramPath = path.get("param");
paramPath.remove();

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 4, 2017

Member

So there's one more test case we need to fix:

try {
} catch (e) {
  e = new TypeError('explain');
}
@xtuc

This comment has been minimized.

Copy link
Member

xtuc commented Aug 6, 2017

That's awesome, thanks.

I think we should also expose some sort of standalone executable. What do you think about that?

@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch from 93d2cc9 to 4902b39 Aug 8, 2017
@MarckK MarckK changed the title Remove unused catch binding [WIP]Remove unused catch binding Aug 8, 2017
@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch 2 times, most recently from f0de374 to 2b5b252 Aug 8, 2017
@MarckK MarckK changed the title [WIP]Remove unused catch binding Remove unused catch binding Aug 8, 2017
@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch from e64ebab to 4e0ec2c Aug 9, 2017
try {
throw 0;
} catch (e) {
let e = new TypeError('New variable declaration, initialization, and assignment; the catch binding is not being referenced and can be removed.');

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 9, 2017

Member

This is actually a runtime error.

This comment has been minimized.

Copy link
@boopathi

boopathi Aug 9, 2017

Member

Yeah. This is fixed in #5980 . and that let e will throw an error now. Can you rebase your branch to 7.0?

CatchClause(path) {
const binding = path.scope.getOwnBinding(path.node.param.name);
if (
binding.constantViolations.filter(el => {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 9, 2017

Member

Why are we filtering?

if (
path.node.param &&
!binding.referenced &&
path.scope.hasBinding(path.node.param.name)

This comment has been minimized.

Copy link
@boopathi

boopathi Aug 9, 2017

Member

I think this is not required here. binding is already defined, and path.scope.hasBinding is going to return true always. Did you face any issues without this check?


visitor: {
CatchClause(path) {
const binding = path.scope.getOwnBinding(path.node.param.name);

This comment has been minimized.

Copy link
@boopathi

boopathi Aug 9, 2017

Member

The binding can be null.

If the actual.js is

try {} catch {}

There is no param, and it'll throw when using binding.something. Can you also add a test case where the catch param is not present.

@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch from 4e0ec2c to a431d15 Aug 15, 2017
@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch from a431d15 to 87a6fbb Aug 23, 2017
@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Aug 23, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4865/

@babel-bot

This comment has been minimized.

Copy link
Collaborator

babel-bot commented Aug 23, 2017

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/4692/

if (path.node.param === null) {
return;
}
const binding = path.scope.getOwnBinding(path.node.param.name);

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 23, 2017

Member

param can be a Pattern, too, so we an only match if it's an Identifier.

}
const binding = path.scope.getOwnBinding(path.node.param.name);
if (
binding.constantViolations.filter(el => {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 23, 2017

Member

Do we really need to filter at all?

) {
return;
}
if (path.node.param && !binding.referenced) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Aug 23, 2017

Member

path.node.param has already been checked above.

@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch 2 times, most recently from 4ac05c5 to b5ea732 Aug 30, 2017
@danez danez closed this Aug 31, 2017
@danez danez reopened this Aug 31, 2017
@danez danez changed the base branch from 7.0 to master Aug 31, 2017
Copy link
Member

jridgewell left a comment

Getting very close.

if (path.node.param === null) {
return;
}
if (t.isObjectPattern(path.node.param)) {

This comment has been minimized.

Copy link
@jridgewell

jridgewell Sep 1, 2017

Member

If it's a pattern, we can just skip this as too much work.

This comment has been minimized.

Copy link
@MarckK

MarckK Sep 1, 2017

Author Member

So, I had previously checked if the param was an identifier and if not exited early--is this what you mean, simply exit early if param not an identifier? (Also: thank you for your code reviews!)

This comment has been minimized.

Copy link
@jridgewell

jridgewell Sep 1, 2017

Member

Yup. We could check all the binding identifiers inside a pattern, but that feels like too much work since the usual pattern is just try {} catch(e) {}

This comment has been minimized.

Copy link
@MarckK

MarckK Sep 14, 2017

Author Member

OK, done. Happy to take on any more advice :-)

@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch from b5ea732 to 6d37b4a Sep 3, 2017
@MarckK MarckK force-pushed the MarckK:remove-unused-catch-binding branch from 6d37b4a to fd5dc18 Sep 11, 2017
@hzoo
hzoo approved these changes Sep 14, 2017
Copy link
Member

hzoo left a comment

code itself looks good, just was thinking about how we want to publish this/users to consume - we can always change it after merge too. Not sure it should be a part of the packages/* - maybe we should move to a codemods/ folder? Ideally, users would just use a npx command to use it, and ultimately lebab would be the tool to do so?

@jridgewell

This comment has been minimized.

Copy link
Member

jridgewell commented Sep 18, 2017

Agree on moving this into a separate folder.

@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 18, 2017

Not sure on the name of the folder but codemods sounds fine for now. We can just name it babel-plugin-codemod-optional-catch-binding then?

@hzoo hzoo merged commit 8dffbf1 into babel:master Sep 19, 2017
3 checks passed
3 checks passed
babel/repl REPL preview is available
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 86.3% (target 80%)
Details
@hzoo

This comment has been minimized.

Copy link
Member

hzoo commented Sep 19, 2017

Nice work @MarckK, going to rename the folder!

@MarckK MarckK deleted the MarckK:remove-unused-catch-binding branch Sep 20, 2017
@lock lock bot added the outdated label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
8 participants
You can’t perform that action at this time.