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

fix named multiline problem pattern parsing #65840

Merged
merged 2 commits into from Jan 8, 2019

Conversation

Projects
None yet
2 participants
@vemoo
Copy link
Contributor

vemoo commented Dec 30, 2018

Before an error was being generated for valid named multiline problem patterns like this:

"problemPatterns": [
    {
        "name": "rustc",
        "patterns": [
            {
                "regexp": "^(warning|warn|error)(?:\\[(.*?)\\])?: (.*)$",
                "severity": 1,
                "code": 2,
                "message": 3
            },
            {
                "regexp": "^[\\s->=]*(.*?):(\\d*):(\\d*)\\s*$",
                "file": 1,
                "line": 2,
                "column": 3
            }
        ]
    }
],

And when an error was generated the pattern was ignored.

This PR fixes that.

@octref octref requested a review from alexr00 Dec 30, 2018

bors bot added a commit to rust-analyzer/rust-analyzer that referenced this pull request Jan 2, 2019

Merge #408
408: vscode problem matcher improvements r=matklad a=vemoo

The problem matcher wasn't working properly and looking at the rustc errors i realized it could be simplified.

I also added a new problem matcher that can be used with https://github.com/passcod/cargo-watch to get the errors in the editor on save. To use it one can create a tasks.json file with:
```json
{
    "version": "2.0.0",
    "tasks": [
        {
            "type": "shell",
            "label": "cargo watch",
            "command": "cargo",
            "isBackground": true,
            "args": [
                "watch",
                "-c"
            ],
            "problemMatcher": [
                "$rustc-watch"
            ]
        }
    ]
}
```
I initially implemented it like this: cff9f62 but i think there's a bug in vscode so i worked around it by copying the pattern for both problem matchers. The first commit can be used if Microsoft/vscode#65840 is merged.


Co-authored-by: Bernardo <berublan@gmail.com>
@alexr00
Copy link
Member

alexr00 left a comment

It looks like the original if check around the error doesn't take into account named multiline problem matchers. Instead of moving the error into this else, the original if should be modified to also check for named and named multiline checked problem matchers.

@vemoo

This comment has been minimized.

Copy link
Contributor Author

vemoo commented Jan 3, 2019

I was going to do that initialy but I think that's part of the reason this bug happened, that the validation is being made twice.
Also I found the if check was hard to read so I tried to simplify it.

My reasoning is the following:

  • If the else branch is reached it doesn't make sense that no error is generated.
  • It doesn't make sense to return a value if an error is generated.
  • Another thing is that Config.NamedMultiLineProblemPattern doesn't exist, only Config.NamedMultiLineCheckedProblemPattern, so we cannot make the same check as for Config.MultiLineProblemPattern.

Another more explicit way to write it while avoiding duplicate checks and being less error prone would be this:

const missingRegExpError = () => {
	this.error(localize('ProblemPatternParser.problemPattern.missingRegExp', 'The problem pattern is missing a regular expression.'));
};
if (Config.NamedMultiLineCheckedProblemPattern.is(value)) {
	return this.createNamedMultiLineProblemPattern(value);
} else if (Config.MultiLineProblemPattern.is(value)) {
	if (Config.MultiLineCheckedProblemPattern.is(value)) {
		return this.createMultiLineProblemPattern(value);
	} else {
		missingRegExpError();
		return null;
	}
} else if (Config.NamedProblemPattern.is(value)) {
	if (Config.NamedCheckedProblemPattern.is(value)) {
		let result = this.createSingleProblemPattern(value) as NamedProblemPattern;
		result.name = value.name;
		return result;
	} else {
		missingRegExpError();
		return null;
	}
} else if (Config.CheckedProblemPattern.is(value)) {
	return this.createSingleProblemPattern(value);
} else {
	missingRegExpError();
	return null;
}

Would that be better?

@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 4, 2019

I see your point about validation being done twice. To keep things concise, how about

    if (Config.NamedMultiLineCheckedProblemPattern.is(value)) {
        return this.createNamedMultiLineProblemPattern(value);
    } else if (Config.MultiLineProblemPattern.is(value) && Config.MultiLineCheckedProblemPattern.is(value)) {
        return this.createMultiLineProblemPattern(value);
    } else if (Config.NamedProblemPattern.is(value) && Config.NamedCheckedProblemPattern.is(value)) {
	let result = this.createSingleProblemPattern(value) as NamedProblemPattern;
	result.name = value.name;
	return result;
    } else if (Config.CheckedProblemPattern.is(value)) {
	return this.createSingleProblemPattern(value);
    } else {
	// this.error line
	return null;
    }
@vemoo

This comment has been minimized.

Copy link
Contributor Author

vemoo commented Jan 4, 2019

I like that, the only things is tha both Config.MultiLineCheckedProblemPattern.is and Config.NamedCheckedProblemPattern.is already check Config.MultiLineProblemPattern.is and Config.NamedProblemPattern.is respectively, so those could be removed.

export namespace MultiLineProblemPattern {
export function is(value: any): value is MultiLineProblemPattern {
return value && Types.isArray(value);
}
}
export type MultiLineCheckedProblemPattern = CheckedProblemPattern[];
export namespace MultiLineCheckedProblemPattern {
export function is(value: any): value is MultiLineCheckedProblemPattern {
let is = false;
if (value && Types.isArray(value)) {
is = true;
value.forEach(element => {
if (!Config.CheckedProblemPattern.is(element)) {
is = false;
}
});
}
return is;
}
}

MultiLineCheckedProblemPattern.is includes the same check that MultiLineProblemPattern.is is doing.
Maybe it should call MultiLineProblemPattern.is?

export namespace NamedProblemPattern {
export function is(value: any): value is NamedProblemPattern {
let candidate: NamedProblemPattern = value as NamedProblemPattern;
return candidate && Types.isString(candidate.name);
}
}
export interface NamedCheckedProblemPattern extends NamedProblemPattern {
/**
* The regular expression to find a problem in the console output of an
* executed task.
*/
regexp: string;
}
export namespace NamedCheckedProblemPattern {
export function is(value: any): value is NamedCheckedProblemPattern {
let candidate: NamedProblemPattern = value as NamedProblemPattern;
return candidate && NamedProblemPattern.is(candidate) && Types.isString(candidate.regexp);
}
}

NamedCheckedProblemPattern.is is already calling NamedProblemPattern.is.

That's why i didn't include the non Checked is calls.

@alexr00

This comment has been minimized.

Copy link
Member

alexr00 commented Jan 8, 2019

Thanks for the contribution and for being so thorough!

@alexr00

alexr00 approved these changes Jan 8, 2019

@alexr00 alexr00 merged commit e531895 into Microsoft:master Jan 8, 2019

1 of 2 checks passed

VS Code #20190104.27 failed
Details
license/cla All CLA requirements met.
Details

@alexr00 alexr00 added this to the December/January 2019 milestone Jan 8, 2019

kjeremy added a commit to kjeremy/rust-analyzer that referenced this pull request Feb 18, 2019

Use named multiline Problem Matcher
Now that Microsoft/vscode#65840 is in the latest
release we can use the first commit from rust-analyzer#408

bors bot added a commit to rust-analyzer/rust-analyzer that referenced this pull request Feb 19, 2019

Merge #858
858: Use named multiline Problem Matcher r=matklad a=kjeremy

Now that Microsoft/vscode#65840 is in the latest release we can use the first commit from #408

Co-authored-by: kjeremy <kjeremy@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment