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

Add ThrowOnRecursive to AnalyzerOptions #5872

Merged
merged 4 commits into from
Sep 2, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions libraries/Microsoft.Bot.Builder.LanguageGeneration/Analyzer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -19,16 +19,20 @@ internal class Analyzer : LGTemplateParserBaseVisitor<AnalyzerResult>
private readonly IExpressionParser _expressionParser;

private readonly Stack<EvaluationTarget> _evaluationTargetStack = new Stack<EvaluationTarget>();

private readonly AnalyzerOptions _analyzerOptions;

/// <summary>
/// Initializes a new instance of the <see cref="Analyzer"/> class.
/// </summary>
/// <param name="templates">Templates.</param>
/// <param name="opt">Options for LG. </param>
public Analyzer(Templates templates, EvaluationOptions opt = null)
/// <param name="analyzerOptions">Options for the analyzer.</param>
public Analyzer(Templates templates, EvaluationOptions opt = null, AnalyzerOptions analyzerOptions = null)
{
Templates = templates;
_templateMap = templates.ToDictionary(t => t.Name);
_analyzerOptions = analyzerOptions;

// create an evaluator to leverage it's customized function look up for checking
var evaluator = new Evaluator(Templates, opt);
Expand All @@ -50,7 +54,23 @@ public Analyzer(Templates templates, EvaluationOptions opt = null)
/// <returns>Analyze result including variables and template references.</returns>
public AnalyzerResult AnalyzeTemplate(string templateName)
{
if (!_templateMap.ContainsKey(templateName) || _evaluationTargetStack.Any(e => e.TemplateName == templateName))
var missingName = !_templateMap.ContainsKey(templateName);
var stackHasName = _evaluationTargetStack.Any(e => e.TemplateName == templateName);

if (_analyzerOptions?.ThrowOnRecursive == true)
{
if (missingName)
{
throw new ArgumentException(TemplateErrors.TemplateNotExist(templateName));
}

if (stackHasName)
{
throw new InvalidOperationException($"{TemplateErrors.LoopDetected} {string.Join(" => ", _evaluationTargetStack.Reverse().Select(e => e.TemplateName))} => {templateName}");
}
}

if (missingName || stackHasName)
{
return new AnalyzerResult();
}
Expand Down Expand Up @@ -210,7 +230,7 @@ private AnalyzerResult AnalyzeExpressionDirectly(Expression exp)
}
else
{
if (!result.TemplateReferences.Contains(templateName))
if (_analyzerOptions?.ThrowOnRecursive == true || !result.TemplateReferences.Contains(templateName))
{
// if template has parameters, just get the template ref without variables.
result.Union(new AnalyzerResult(templateReferences: this.AnalyzeTemplate(templateName).TemplateReferences));
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
// Copyright (c) Microsoft Corporation. All rights reserved.
// Licensed under the MIT License.

using System;
using System.Collections.Generic;

namespace Microsoft.Bot.Builder.LanguageGeneration
{
/// <summary>
/// Options for analyzing LG templates.
/// </summary>
public class AnalyzerOptions
{
private readonly string _throwOnRecursive = "@throwOnRecursive";

/// <summary>
/// Initializes a new instance of the <see cref="AnalyzerOptions"/> class.
/// </summary>
public AnalyzerOptions()
{
ThrowOnRecursive = null;
}

/// <summary>
/// Initializes a new instance of the <see cref="AnalyzerOptions"/> class.
/// </summary>
/// <param name="opt">Instance to copy analyzer settings from.</param>
public AnalyzerOptions(AnalyzerOptions opt)
{
ThrowOnRecursive = opt.ThrowOnRecursive;
}

/// <summary>
/// Initializes a new instance of the <see cref="AnalyzerOptions"/> class.
/// </summary>
/// <param name="optionsList">List of strings containing the options from an LG file.</param>
public AnalyzerOptions(IList<string> optionsList)
{
if (optionsList != null)
{
foreach (var option in optionsList)
{
if (!string.IsNullOrWhiteSpace(option) && option.Contains("="))
{
var index = option.IndexOf('=');
var key = option.Substring(0, index).Trim();
var value = option.Substring(index + 1).Trim();

if (key.Equals(_throwOnRecursive, StringComparison.OrdinalIgnoreCase))
{
if (value.ToLowerInvariant() == "true")
{
ThrowOnRecursive = true;
}
}
}
}
}
}

/// <summary>
/// Gets or sets a value determining if recursive calls throw an exception.
/// </summary>
/// <value>
/// When true, throw an exception if a recursive call is detected.
/// </value>
public bool? ThrowOnRecursive { get; set; } = null;

/// <summary>
/// Merge a incoming option to current option. If a property in incoming option is not null while it is null in current
/// option, then the value of this property will be overwritten.
/// </summary>
/// <param name="opt">Incoming option for merging.</param>
/// <returns>Result after merging.</returns>
public AnalyzerOptions Merge(AnalyzerOptions opt)
{
var properties = typeof(AnalyzerOptions).GetProperties();
foreach (var property in properties)
{
if (property.GetValue(this) == null && property.GetValue(opt) != null)
{
property.SetValue(this, property.GetValue(opt));
}
}

return this;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -320,11 +320,13 @@ public IList<object> ExpandTemplate(string templateName, object scope = null, Ev
/// Analyzes a template to get the static analyzer results including variables and template references.
/// </summary>
/// <param name="templateName">Template name to be evaluated.</param>
/// <param name="analyzerOptions ">Options used to determine behavior of the analyzer.</param>
/// <returns>Analyzer result.</returns>
public AnalyzerResult AnalyzeTemplate(string templateName)
public AnalyzerResult AnalyzeTemplate(string templateName, AnalyzerOptions analyzerOptions = null)
{
CheckErrors();
var analyzer = new Analyzer(this);

var analyzer = new Analyzer(this, this.LgOptions, analyzerOptions);
return analyzer.AnalyzeTemplate(templateName);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,5 +9,5 @@
- ${wPhrase()}

> self loop
# shouldFail(x)
- ${shouldFail(x)}
# selfLoop(x)
- ${selfLoop(x)}
Original file line number Diff line number Diff line change
Expand Up @@ -289,6 +289,22 @@ public void TestLoopDetected()
var lgFile = GetTemplates("LoopDetected.lg");
var exception = Assert.Throws<InvalidOperationException>(() => lgFile.Evaluate("wPhrase"));
Assert.Contains(TemplateErrors.LoopDetected, exception.Message);

// Without ThrowOnRecursive does not throw exception when loop is detected
var wPhraseResult = lgFile.AnalyzeTemplate("wPhrase");
Assert.Equal("welcome_user", wPhraseResult.TemplateReferences[0]);
Assert.Equal("wPhrase", wPhraseResult.TemplateReferences[1]);

var selfLoopResult = lgFile.AnalyzeTemplate("selfLoop");
Assert.Equal("selfLoop", selfLoopResult.TemplateReferences[0]);
Assert.Equal("x", selfLoopResult.Variables[0]);

// ThrowOnRecursive throws InvalidOperationException
exception = Assert.Throws<InvalidOperationException>(() => lgFile.AnalyzeTemplate("wPhrase", new AnalyzerOptions() { ThrowOnRecursive = true }));
Assert.Contains(TemplateErrors.LoopDetected, exception.Message);

exception = Assert.Throws<InvalidOperationException>(() => lgFile.AnalyzeTemplate("selfLoop", new AnalyzerOptions() { ThrowOnRecursive = true }));
Assert.Contains(TemplateErrors.LoopDetected, exception.Message);
}

[Fact]
Expand Down