Fix error cases of conditions and conditional parameters #5704

JanKrivanek · 2022-11-29T20:42:10Z

Problem

Fixes #5647
Fixes #5646
Some malformed expressions are evaluated as if without issues (e.g. 'Param = false')

Solution

Improves the error detection and reporting for malformed expressions evaluation
Improves error reporting of authoring issues for conditional parameters (malformed conditions or missing default parameters)
Removes the disabled parameters from the configured parameters (as if they were never defined on a template)

Case of missing default values

Missing default value on a parameter that is not required is always suspitious (and should likely be flaged by the template validation)
It is hard to guess what the sensible default should be in such case - even for boolean parameters (shown by unit case with inverted enabling parameter A_disabled). It's probably best bet to flag such a case via error. Current error seemes to be clear enough:

Failed to create template.
Details: Failed to evaluate condition IsEnabled on parameter A (condition text: A_enable, evaluation error: String 'A_enable' was not recognized as a valid Boolean.) - condition might be malformed or referenced parameters do not have default nor explicit values." to contain "xFailed to evaluate condition IsEnabled on parameter A

On a second thought - the condition within template are evaluated via different evaluator (CPP vs CPP2) and behaves as if default value was specified (thanks to default value of expression https://github.com/dotnet/templating/blob/main/src/Microsoft.TemplateEngine.Core/Expressions/Cpp/CppStyleEvaluatorDefinition.cs#L533 and default value when negating subexpression https://github.com/dotnet/templating/blob/main/src/Microsoft.TemplateEngine.Core/Expressions/Cpp/Scope.cs#L61).

So the question is if we want:
a) Consistency, but hide possible authoring issues.
b) Fail fast and clearly for authoring issues but lose consistency with instantiation.

Checks:

Added unit tests
Added #nullable enable to all the modified files ?

vlada-shubina · 2022-12-01T08:55:41Z

First thought on default values: imo defining default values for boolean is superfluous and will easily become authoring burden.

They should only be defined in case different from common meaning: defaultValue: false, defaultIfOptionWithoutValue: true.
We may discuss other types separately.

vlada-shubina

Let's discuss couple of points:

consider keeping to the generator scope to remove parameters or not. If so API changes are not required.
let's consider implicit default values for boolean instead and extract supporting default values for other types in a separate issue. Removing them will probably be a "breaking" change. A_disabled seems to be a unique scenario, and in this case likely defaultValue will be specified to true if desired.

src/Microsoft.TemplateEngine.Abstractions/ITemplate.cs

src/Microsoft.TemplateEngine.Edge/Template/TemplateCreator.cs

src/Microsoft.TemplateEngine.Orchestrator.RunnableProjects/ConfigModel/TemplateConfigModel.cs

vlada-shubina

Looks good, thank you!

Just wondering, it it makes sense to add the integration test(s) for cases we explicitly faced to have them as the reference.

Follow up: uncomment those tests in sdk
https://github.com/dotnet/sdk/blob/66738868dfe1bb818a960d8bf501b0b6e4aa86f6/src/Tests/dotnet-new.Tests/DotnetNewInstantiateTests.Approval.cs#L688

https://github.com/dotnet/sdk/blob/66738868dfe1bb818a960d8bf501b0b6e4aa86f6/src/Tests/dotnet-new.Tests/DotnetNewInstantiateTests.Approval.cs#L737

vlada-shubina · 2022-12-13T13:23:06Z

src/Microsoft.TemplateEngine.Orchestrator.RunnableProjects/ParameterConverter.cs

+            {
+                string s when string.IsNullOrEmpty(s) => null,
+                string s when s.Equals("bool", StringComparison.OrdinalIgnoreCase) => false.ToString(),
+                string s when s.Equals("choice", StringComparison.OrdinalIgnoreCase) => string.Empty,


shouldn't it be null for choice?

null for choice means that default value should be used.
If defualt would be null as well - it would be recognized as error: https://github.com/dotnet/templating/blob/main/src/Microsoft.TemplateEngine.Orchestrator.RunnableProjects/ParameterConverter.cs#L147-L148

string.Empty is explicit unset - sounds as reasonable default

test/Microsoft.TemplateEngine.Edge.UnitTests/TemplateCreatorTests.cs

vlada-shubina · 2022-12-13T13:25:41Z

src/Microsoft.TemplateEngine.Orchestrator.RunnableProjects/ConfigModel/ParameterSymbol.cs

@@ -62,6 +62,11 @@ internal ParameterSymbol(string name, JObject jObject, string? defaultOverride)
                DefaultIfOptionWithoutValue = "true";
            }

+            if (DefaultValue == null && IsRequired != true)


is IsRequired needed here?
does it change any behavior?

IsRequired indicates that value needs to be specified. Dafult is not needed. So it can technically be removed here from the condition without any impact. I just wanted the code not to create impression that the Default is created and applied even for required parameters.

JanKrivanek · 2022-12-13T15:04:06Z

Looks good, thank you!

Just wondering, it it makes sense to add the integration test(s) for cases we explicitly faced to have them as the reference.

Follow up: uncomment those tests in sdk https://github.com/dotnet/sdk/blob/66738868dfe1bb818a960d8bf501b0b6e4aa86f6/src/Tests/dotnet-new.Tests/DotnetNewInstantiateTests.Approval.cs#L688

https://github.com/dotnet/sdk/blob/66738868dfe1bb818a960d8bf501b0b6e4aa86f6/src/Tests/dotnet-new.Tests/DotnetNewInstantiateTests.Approval.cs#L737

Yes, Yes
It's the InstantiateAsync_ConditionalParametersRequiredOverwrittenByDisabled test added in this PR - with all possible variations of parameter values combinations (and their omission)

JanKrivanek · 2022-12-13T15:07:59Z

/backport to release/7.0.2xx

github-actions · 2022-12-13T15:08:35Z

Started backporting to release/7.0.2xx: https://github.com/dotnet/templating/actions/runs/3686669324

github-actions · 2022-12-13T15:08:42Z

@JanKrivanek backporting to release/7.0.2xx failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Fix error cases of conditions and conditional parameters
.git/rebase-apply/patch:186: trailing whitespace.
    
.git/rebase-apply/patch:187: trailing whitespace.
    
.git/rebase-apply/patch:199: trailing whitespace.
        
.git/rebase-apply/patch:201: trailing whitespace.
        
.git/rebase-apply/patch:203: trailing whitespace.
        
warning: squelched 33 whitespace errors
warning: 38 lines add whitespace errors.
Using index info to reconstruct a base tree...
M	src/Microsoft.TemplateEngine.Abstractions/PublicAPI.Unshipped.txt
M	src/Microsoft.TemplateEngine.Core/Expressions/Shared/SharedEvaluatorDefinition.cs
M	src/Microsoft.TemplateEngine.Core/PublicAPI.Shipped.txt
M	src/Microsoft.TemplateEngine.Edge/Template/TemplateCreator.cs
M	src/Microsoft.TemplateEngine.Orchestrator.RunnableProjects/RunnableProjectConfig.cs
Falling back to patching base and 3-way merge...
Auto-merging src/Microsoft.TemplateEngine.Orchestrator.RunnableProjects/RunnableProjectConfig.cs
Auto-merging src/Microsoft.TemplateEngine.Edge/Template/TemplateCreator.cs
Auto-merging src/Microsoft.TemplateEngine.Core/PublicAPI.Shipped.txt
CONFLICT (content): Merge conflict in src/Microsoft.TemplateEngine.Core/PublicAPI.Shipped.txt
Auto-merging src/Microsoft.TemplateEngine.Core/Expressions/Shared/SharedEvaluatorDefinition.cs
Auto-merging src/Microsoft.TemplateEngine.Abstractions/PublicAPI.Unshipped.txt
CONFLICT (content): Merge conflict in src/Microsoft.TemplateEngine.Abstractions/PublicAPI.Unshipped.txt
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Fix error cases of conditions and conditional parameters
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

github-actions · 2022-12-13T15:08:43Z

@JanKrivanek an error occurred while backporting to release/7.0.2xx, please check the run log for details!

Error: git am failed, most likely due to a merge conflict.

* Fix error cases of conditions and conditional parameters * Add default values assignment and tests * Fix tests * Fix PR suggestions

…] to release/7.0.2xx (#5765) * Fix error cases of conditions and conditional parameters (#5704) * Fix error cases of conditions and conditional parameters * Add default values assignment and tests * Fix tests * Fix PR suggestions * Fix contract * Extend macros contract and improve coalesce macro (#5817) * Improve coalesce macro * Add test * Add comments * Fix contracts

Fix error cases of conditions and conditional parameters

ef4af06

JanKrivanek requested a review from a team as a code owner November 29, 2022 20:42

vlada-shubina reviewed Dec 1, 2022

View reviewed changes

JanKrivanek added 3 commits December 5, 2022 14:00

Add default values assignment and tests

7e05470

Fix tests

5002a0c

Fix PR suggestions

f100220

JanKrivanek requested a review from vlada-shubina December 5, 2022 15:51

vlada-shubina approved these changes Dec 13, 2022

View reviewed changes

JanKrivanek merged commit b26cc3b into dotnet:main Dec 13, 2022

JanKrivanek deleted the conditional-params-fixing branch December 13, 2022 15:06

JanKrivanek mentioned this pull request Dec 14, 2022

Cherry pick [Fix error cases of conditions and conditional parameters] to release/7.0.2xx #5764

Closed

This was referenced Dec 14, 2022

Cherry pick [Fix error cases of conditions and conditional parameters] to release/7.0.2xx #5765

Merged

Improve VariableCollection contract and CoalesceMacro to handle default values gracefully #5776

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix error cases of conditions and conditional parameters #5704

Fix error cases of conditions and conditional parameters #5704

JanKrivanek commented Nov 29, 2022 •

edited

Loading

vlada-shubina commented Dec 1, 2022

vlada-shubina left a comment •

edited

Loading

vlada-shubina left a comment

vlada-shubina Dec 13, 2022

JanKrivanek Dec 13, 2022

vlada-shubina Dec 13, 2022

JanKrivanek Dec 13, 2022

JanKrivanek commented Dec 13, 2022

JanKrivanek commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

Fix error cases of conditions and conditional parameters #5704

Fix error cases of conditions and conditional parameters #5704

Conversation

JanKrivanek commented Nov 29, 2022 • edited Loading

Problem

Solution

Case of missing default values

Checks:

vlada-shubina commented Dec 1, 2022

vlada-shubina left a comment • edited Loading

Choose a reason for hiding this comment

vlada-shubina left a comment

Choose a reason for hiding this comment

vlada-shubina Dec 13, 2022

Choose a reason for hiding this comment

JanKrivanek Dec 13, 2022

Choose a reason for hiding this comment

vlada-shubina Dec 13, 2022

Choose a reason for hiding this comment

JanKrivanek Dec 13, 2022

Choose a reason for hiding this comment

JanKrivanek commented Dec 13, 2022

JanKrivanek commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

github-actions bot commented Dec 13, 2022

JanKrivanek commented Nov 29, 2022 •

edited

Loading

vlada-shubina left a comment •

edited

Loading