fix named multiline problem pattern parsing

[vemoo]

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.

[alexr00]

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]

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]

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]

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.

https://github.com/Microsoft/vscode/blob/ac140c084c6de6a947f116b2ed24a7634a625306/src/vs/workbench/parts/tasks/common/problemMatcher.ts#L618-L639

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

https://github.com/Microsoft/vscode/blob/ac140c084c6de6a947f116b2ed24a7634a625306/src/vs/workbench/parts/tasks/common/problemMatcher.ts#L594-L614

NamedCheckedProblemPattern.is is already calling NamedProblemPattern.is.

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

[alexr00]

Thanks for the contribution and for being so thorough!