GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — Add code fix to remove unreachable code · Pull Request #24028 · microsoft/TypeScript

Conversation

Deleted user (ghost) commented May 10, 2018

Copy link

Copy Markdown

Mentioned in #24026

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

ghost requested review from Andrew Casey (amcasey) and Mohamed Hegazy (mhegazy)

May 10, 2018 17:21

ghost force-pushed the codeFixUnreachableCode branch from 7b9ac54 to 539a43d Compare

May 10, 2018 17:26


          Add code fix to remove unreachable code

88b7948

ghost force-pushed the codeFixUnreachableCode branch from 539a43d to 88b7948 Compare

May 10, 2018 17:27

Mohamed Hegazy (mhegazy) reviewed

May 10, 2018

View reviewed changes

Comment thread

src/services/codefixes/fixUnreachableCode.ts Outdated

		const container = (isBlock(statement.parent) ? statement.parent : statement).parent;
		switch (container.kind) {
		case SyntaxKind.IfStatement:
		return (container as IfStatement).elseStatement ? statement : container as IfStatement;

Mohamed Hegazy (mhegazy) May 10, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

if the statement was not in a block, that will leave the the ifStatement syntactically incorrect.. e.g.

    if (false) console.log();
    else console.log();

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) reviewed

May 10, 2018

View reviewed changes

Comment thread

src/services/codefixes/fixUnreachableCode.ts Outdated

		case SyntaxKind.ForStatement:
		return container as WhileStatement \| ForStatement;
		default:
		return statement;

Mohamed Hegazy (mhegazy) May 10, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

you want to return all statemetns from this one untill the end of the block, e.g.:

function f() { 
   return 0;

  // both should be removed, error is only on the first.
   var x = 0;
   console.log(x);
}

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Andy Hanson added 2 commits

May 10, 2018 13:03


          Code review

ce03cf6


          Merge branch 'master' into codeFixUnreachableCode

8a1dd53

Mohamed Hegazy (mhegazy) approved these changes

May 10, 2018

View reviewed changes

Klaus Meinhardt (ajafff) reviewed

May 10, 2018

View reviewed changes

Comment thread

src/services/codefixes/fixUnreachableCode.ts Outdated

		break;
		default:
		if (isBlock(statement.parent)) {
		split(sliceAfter(statement.parent.statements, statement), s => !isFunctionDeclaration(s), (start, end) => changes.deleteNodeRange(sourceFile, start, end));

Klaus Meinhardt (ajafff) May 10, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

You probably also want to preserve type alias declarations, interfaces, const enums, type only namespaces, var statements, ...

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

mhegazy reacted with thumbs up emoji

All reactions

👍 1 reaction


          Preserve more kinds of statements

ccfcadf

ghost mentioned this pull request

May 10, 2018

Add code fix to remove unused label #24037

Merged

Mohamed Hegazy (mhegazy) reviewed

May 10, 2018

View reviewed changes

Comment thread

src/services/codefixes/fixUnreachableCode.ts

		// Don't remove statements that can validly be used before they appear.
		return !isFunctionDeclaration(s) && !isPurelyTypeDeclaration(s) &&
		// `var x;` may declare a variable used above
		!(isVariableStatement(s) && !(getCombinedNodeFlags(s) & (NodeFlags.Let \| NodeFlags.Const)) && s.declarationList.declarations.some(d => !d.initializer));

Mohamed Hegazy (mhegazy) May 10, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

if it is a var statement it make sure it is not ambient.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) May 10, 2018

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

If it's ambient, it should not have an initializer so we won't remove it, right?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) May 10, 2018

Copy link

Copy Markdown

Contributor

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

declare let x; would be removed, right?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Deleted user (ghost) May 10, 2018

Copy link

Copy Markdown

Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Oh, right.
Actually, I can't think of a way a declare statement would be unreachable?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Mohamed Hegazy (mhegazy) approved these changes

May 10, 2018

View reviewed changes

ghost merged commit 6ae4d3a into master

May 10, 2018

ghost deleted the codeFixUnreachableCode branch

May 10, 2018 23:44

Andrew Casey (amcasey) reviewed

May 11, 2018

View reviewed changes

Andrew Casey (amcasey) left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

A few questions.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

tests/cases/fourslash/codeFixUnreachableCode_if.ts

		newFileContent:
		`
		// No good way to delete just the 'if' part
		if (false) { } else b;

Andrew Casey (amcasey) May 11, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

In a perfect world, the output would be b;?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/fixUnreachableCode.ts

		case SyntaxKind.IfStatement:
		if ((container as IfStatement).elseStatement) {
		if (isBlock(statement.parent)) {
		changes.deleteNodeRange(sourceFile, first(statement.parent.statements), last(statement.parent.statements));

Andrew Casey (amcasey) May 11, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Don't we want to eliminate the whole else-clause?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/fixUnreachableCode.ts

		}

		function shouldRemove(s: Statement): boolean {
		// Don't remove statements that can validly be used before they appear.

Andrew Casey (amcasey) May 11, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I'm missing something - why would it be reported as unused if it's used above?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

src/services/codefixes/fixUnreachableCode.ts

		split(sliceAfter(statement.parent.statements, statement), shouldRemove, (start, end) => changes.deleteNodeRange(sourceFile, start, end));
		}
		else {
		changes.deleteNode(sourceFile, statement);

Andrew Casey (amcasey) May 11, 2018

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Will a switch case ever be reported as unused? Does that need special handling? What about a catch block?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Microsoft (microsoft) locked and limited conversation to collaborators

Jul 31, 2018

This pull request was closed.

Reviewers

No reviews

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

3 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

Deleted user (ghost) commented May 10, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Mohamed Hegazy (mhegazy) May 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Andrew Casey (amcasey) left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Mohamed Hegazy (mhegazy) May 10, 2018 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.