afscrome / ironvelocity Goto Github PK

Investigate whether we can store local variables using a slotted array. This would improve the performance of retrieving variables to a single array lookup rather than having to do a a dictionary lookup by key.

Many uses of DynamicMetaObject.RuntimeType should be replaced by DynamicMetaObject.LimitType to prevent null references. IF code requires that the dynamic metaobject has a value, it should explicitly check the HasValue property.

Look into allowing string concatenation with '+'. This was enabled in java velocity with VELOCITY-148.

Velocity is often used for HTML templates, or other scenarios in which whitespace is generally insignificant. In some cases we can significantly reduce the size of the template output by trimming out whitespace in the template at parse time using the following rules:

• Completely remove any sequence of (WHITESPACE | NEWLINE)+ at the beginning or end of a template.
• Replace any sequence of (WHITESPACE | NEWLINE) NEWLINE (WHITESPACE | NEWLINE)* with a single newline
• Replace any sequence of (WHITESPACE)+ with a single space

This should be made optional as there may be scenarios in which whitespace is significant and shouldn't be modified at all.

Add support for indexers ala velocity 1.7 e.g. $foo.Stuff["key"]

Implement a velocity Parser to remove the dependency on NVelocity so we can support .Net Core.

Review all the binders to ensure they are consistent with each other.

• Handling of null target
• Handling of unresolveable result
• Binding Restrictions
• Handling of errorSuggestion
• Logging of failures
• Deferring of target AND arguments

Attempting to set a property with a private setter currently succeeds. It should fail, logging an error. May also affect internal / protected setters.

Trying to call set on a getter only property should be ignored. It currently throws an exception like the below:

#set($myObj.Example = "test")

public class MyObj
{
    public string Example {get; private set;}
}

In a foreach directive, the current item & $velocityCount variables persist after the end of the foreach loop

This doesn't seem to be affecting inner foreach directives, only the outer one. (Since the ShouldSupportCurrentItemBeingRedefinedInNestedForeach test in BasicForeachTests.cs passes). I assume the reason for this is that we try to assign null to the context item, which we ignore. Need to find a way to explicitly remove an item from the context and use that.

[Test]
public void ShouldNotPersistCurrentItemOrVelocityCountOutsideForeach()
{
    var input = "#foreach($x in [7,8,1]) #end";
    var result = ExecuteTemplate(input);

    Assert.That(result.Context, Is.Empty);
}

Review all uses of Type.IsValueType, and determine if they should be replaced with calls to ReflectionHelper.IsNullableType()

Keeping the two separate is just fighting the DLR - merge the two together to keep things simpler.

Currently there are a few NVelocity specific features embedded in the core, we should pull these out and allow these to be extensions instead.

• to_squote
• to_quote
• dictionary strings
• special handling for properties of object that is typeof(ENUM)
• Properties on dictionary act as indexers.
• ??Equivalence of "Count" and "Length" on array types??

Velocity lists can have trouble interoping with C# code which take in generic collections. Consider the following:

void Foo(IEnumerable<string> values);
void Bar(params string[] values)

#set($x = ["hello", "world]")
1. $y.Bar("hello", $world)
2. $y.Foo($x)
3. $y.Bar($x)

1 works, but 2 and 3 don't. Look into how we can support implicit conversion of velocity lists to IEnumerable<T>, ICollection<T>, IReadOnlyCollection<T>

(Based on https://community.telligent.com/community/f/554/t/1140977 )

VelocityInvokeMemberBinder does not work with ExpandoObject. This is because ExpandoObject works by a GetMember operation, followed by an Invoke on the retrieved member. This fails because FallbackInvoke is not implemented in VelocityInvokeMemberBinder .

In FallbackInvoke, the target should be a delegate (Func, Action, custom deletage), which should be invoked with the given arguments (provided they're compatible)

public void ShouldInvokeMemberOnDynamicObject()
{
    dynamic input = new ExpandoObject();
    input.Double = new Func<int,int>(x => x * 2);

    var result = test(input, "Double", 123);
    Assert.That(result, Is.EqualTo(246));
}

Related to #45.

Should we add support for velocimacros? Personally I'm not a huge fan of velocimacros as they have a number of unexpected nuances that can cause problems / confusion.

In the previous NVelocity based implementation of IronVelocity, I implemented macro support purely to get the regression tests to pass, although the implementation wasn't complete. Some of this could be reused if support is reintroduced, although I'd rather not introduce support purely to make some tests happy.

A lot of uses or TemporaryVariableScopeExrpession are effectively implementing null propagation - there may be a more efficient way of dealing with this specific scenario.

One such approach may be to replace the use of Blocks with Lambda expressions.

The following is valid in NVelocity, but looks like it should be a syntax error. This is causing problems with the Antlr based parser. Investigate / decide whether this is a bug in nvelocity, and if we should maintain backwards comparability.

#set($x = 123)
#set($y = $$x)
#set($z = #$x)

$x || $y ||$z

Expected: 123 ||  ||  
Actual:   123 || 123 || 123

3 of the Boxing tests only pass when Static Typing is enabled. From a quick glance, I'm guessing this is due to the struct value being boxed before the invoke member binder is executed against it.

In Velocity, whitespace before a #set directive is eaten, In IronVelocity, only trailing whitespace is swallowed.

Input: \t #set($x = 123)Hello
Expected Output: Hello
Current Output : \t Hello

Add tests to ensure that template execution correctly performs invocations, & binary operations on dynamic types.

IronVelocity is currently inconsistent as to whether operator overloads are supported. They are supported in relational operations, but not in mathematical operations.

For consistency, we should either support operator overloads everywhere, or nowhere.

Velocity 1.7 allows you to use two quote characters in a row as a way of escaping quotes e.g. 'John''s coat' would be the string 'John's coat. This would not introduce any grammar ambiguities, nor would it change the meaning of any existing strings.

It would be awesome to support this in IronVelocity.

The string '#custom ( parses in NVelocity (Assuming that custom is an undefined directive) If the directive is defined it produces a parse error. In Ironvelocity it always produces a parse error even if the directive is undefined.

Need to do some more work around error messages from the parser to ensure they are actionable to fix. So far I've identified the following:

• Unexpected #end should provide a clear error e.g. "Unexpected #end" + line number
• Missing #end after #if or custom block directive should indicate the name & position of the block directive missing the #end
• HandleFailures in AntlrVelocityParser should throw a more appropriate exception type.

Currently the get member binder will match any instance fields. This is different to the java behaviour

Note: References to instance variables in a template are not resolved. Only references to the attribute equivalents of JavaBean getter/setter methods are resolved (i.e. $foo.Name does resolve to the class Foo's getName() instance method, but not to a public Name instance variable of Foo).
http://velocity.apache.org/engine/devel/user-guide.html#Case_Substitution

Need to confirm the behaviour with NVelocity. If NVelocity doesn't access fields, we should definitely fix this. If we remove binding to instance fields, should we support binding to constant fields? public const string Foo = "test"

Add native support for dictionaries (as in velocity 1.7) e.g. { 'key' : 'value', 'other': 123} .

This would allow for the obsoletion / removal of the nvelocity dictionary strings "%{}" which have some rather funky characteristics.

Currently there are big differences between velocity, and IronVelocity in terms of how they handle escaping

• In Velocity, the entire reference/directive is escaped. The string \$foo.bar( produces a syntax error because $foo.bar( produces a syntax error. It also means that the sequence \$foo.bar() causes $foo.bar to be evaluated. In IronVelocity, escaping is a more traditional escape character. The sequence \$ is treated as a textual $, and not the start of a reference.
• Iron Velocity does not support the syntax $\foo, $\!foo at all

Personally, I feel the way in which velocity does escaping is very confusing, I'd prefer to keep the simple escape sequence aproach currently implemented

For reference types, DefaultExpression is always null. Value types are sealed, so the default value will be identical for a given DefaultExpression.

If someone provides an integer literal that overflows or underflows, a parser exception occurs
#set($x=999999999)

Either provide a clearer error, or automatically treat this value as a floating point number.

Interpolatd strings don't correctly handle null or undefined references. See the following two tests:

• ShouldProcessInterpolatedStringWithMixedTextAndUndefinedReference
• ShouldProcessInterpolatedStringWithUndefinedReference

Allow directives to be formally specified with curley braces to allow for text to preceed a directive without adding whitespace. e.g. #{if($condition)}something#{else}otherthing#{end}

There is currently very little test coverage around the global variable support.

Look at using the TestFixture attribute to allow TemplateExecution tests to run with the initial context as both global variables, and as normal variables. See Parser.ReferenceTests for an example. (N.B. some tests may not be valid for using globals, specifically those that assign a variable that was provided in the context)

NVelocity uses the following approaches to compare two objects used in a relational expression.

• Reference equality
• Object.Equals()
• IComparable / IEquatable

Iron Velocity on the other hand relies purely on operator overloads. For primitive objects, this has no effect on the end result as most primitives implement both operator overloads & IComparable/IEquatable. There may however be regressions in comparisons of custom objects if we don't implement that support.

What methods should IronVelocity use for relational comparisons by default? (N.B. this behaviour can be customised by consumers by replacing the ComparisonOperationBinder if there are specific needs by specific consumers).

Look at adding support for break & continue statements in foreach.

Need to think about whether any special handling is needed with templated foreach

Currently we compile the templates This has a startup cost, which then leads to further startup costs on the first execution whilst the template is JIT compiled. We should look at whether we can interpret these templates to speed this up. This could be done either using the DLR scripting bits as used by IronPython. Alternatively depending on dotnet/corefx#3244 this support may be built in.

A hybrid aproach could also be adopted to compromise on startup time vs raw performance - interpret a template the first X executions to optimise for startup speed. After X executions, compile in the background then swap to use the compiled template. I believe this is alreadyd one in the DLR scripting bits, but would need to check.

BinderFactory mixes two seperate concerns - creating of binders, and sharing binders. This should be split into two classes.

Trying to do a GetIndex on a class with the below two indexer declarations with an integer argument is being resolved as ambigious - in both cases it shoudl be non ambigious (resolving to the indexer without a params array)

public int this[int  i] => i;
public string this[params long[] values] => string.Join(", ", values);

public long this[long  i] => i;
public string this[params long[] values] => string.Join(", ", values);

If you have a global variable that is a private class, accessing members of the variable will work fine in dynamic mode, but fail in static typed mode. This is because in dynamic mode, the member access happens in a DynamicMethod, which does not perform accessibility checks. In static type mode however, the member access occurs in a DynamicAssembly which does do accessibility checking. So the JIT will fail to compile the method.

See https://msdn.microsoft.com/en-us/library/9syytdak.aspx#Dynamic_Methods_Associated_with_Existing_Assemblies

Investigate how to add support awaiting on asynchronous calls in templates. The roslyn scripting APIs may be one route to do this.

Allow for the definition of #[[...]] which allows the specification of text that is not parsed, and treated verbatim.
#[[This is not parsed $(, and so won't produce parser errors, even if it is syntaticly invalid]]

Trying to call set on a getter only property should be ignored. It currently throws an exception like the below:

#set($myObj.Example = "test")

public class MyObj
{
    public string Example {get {return "something";}}
}

System.ArgumentException : Expression must be writeable
Parameter name: left
at System.Linq.Expressions.Expression.RequiresCanWrite(Expression expression, String paramName)
at System.Linq.Expressions.Expression.Assign(Expression left, Expression right)
at IronVelocity.Binders.VelocitySetMemberBinder.FallbackSetMember(DynamicMetaObject target, DynamicMetaObject value, DynamicMetaObject errorSuggestion) in C:\Alex\Documents\GitHub\IronVelocity\IronVelocity\Binders\VelocitySetMemberBinder.cs:line 39
at System.Dynamic.DynamicMetaObject.BindSetMember(SetMemberBinder binder, DynamicMetaObject value)
at System.Dynamic.SetMemberBinder.Bind(DynamicMetaObject target, DynamicMetaObject[] args)
at System.Dynamic.DynamicMetaObjectBinder.Bind(Object[] args, ReadOnlyCollection`1 parameters, LabelTarget returnLabel)
at System.Runtime.CompilerServices.CallSiteBinder.BindCore[T](CallSite`1 site, Object[] args)
at System.Dynamic.UpdateDelegates.UpdateAndExecute2[T0,T1,TRet](CallSite site, T0 arg0, T1 arg1)
at lambda_method(Closure )
at IronVelocity.Tests.Utility.BinderTests[T](CallSiteBinder binder, Object[] args) in C:\Alex\Documents\GitHub\IronVelocity\IronVelocity.Tests\Utility.cs:line 130
at IronVelocity.Tests.Utility.BinderTests(CallSiteBinder binder, Object[] args) in C:\Alex\Documents\GitHub\IronVelocity\IronVelocity.Tests\Utility.cs:line 120
at IronVelocity.Tests.Binders.SetMemberBinderTests.TestAssignmentOnReferenceType[TTarget,TValue](TTarget input, String memberName, TValue value) in C:\Alex\Documents\GitHub\IronVelocity\IronVelocity.Tests\Binders\SetMemberBinderTests.cs:line 66
at IronVelocity.Tests.Binders.SetMemberBinderTests.ClassSetGetterOnlyPropertyIgnored() in C:\Alex\Documents\GitHub\IronVelocity\IronVelocity.Tests\Binders\SetMemberBinderTests.cs:line 57