CompiledDataBinder: "Body of catch must have the same type as body of try" exception

[ChairmanMawh]

Not too sure what to do with this one (not a big user of expressions)..

This console app reproduces it with 1.3.5, 0.2.12, and .net 7

        static bool HandleError(DataValidationContext validationContext)
        {
            return true;
        }

        public class X{
            public string UniqueIdentifier { get; set; }
            public DateTime RecordDate { get; set; }
            public decimal? Data1 { get; set; }
        }

        public static void Main(string[] args)
        {
            var csvStr = @"Z, Y, ID, X, W, V, A1, A2
""A"", ""B"", ""C"", ""D"", ""E"", 01/01/2000, 12.345, 67.890"u8;
            var schema = "ID>UniqueIdentifier:string,V>RecordDate:DateTime{dd/MM/yyyy},A1>Data1:decimal?";

            var opts = new CsvDataReaderOptions { Schema = new CsvSchema(Sylvan.Data.Schema.Parse(schema)), HasHeaders = true };
            using var csv = CsvDataReader.Create(new StreamReader(new MemoryStream(csvStr.ToArray())), opts).ValidateSchema(HandleError);

            var binderOpts = new DataBinderOptions() { BindingMode = DataBindingMode.Any };
            var l = csv.GetRecords<X>(binderOpts).ToList(); //boom
    }

[ChairmanMawh]

Pumelling a couple of AIs eventualy got to the suggestion to change the code to:

var catchExpr = Expression.Catch(
			exParam,
			Expression.Block(
				Expression.Throw(Expression.New(exCtor!, idxVar, exParam)),
				Expression.NewArrayInit(typeof(object)) // Return an empty array of objects
			)
		); 

which seems to stop the exception throwing, but no idea if it's right..

[ChairmanMawh]

Oof, feels like hitting a moving target here. I'd had an earlier sugestion from Bing AI to change to:

var catchExpr = Expression.Catch(exParam, Expression.Block(Expression.Throw(Expression.New(exCtor!, idxVar, exParam)), Expression.Default(typeof(void))));

which I thought had worked, but then turned out not to, hence the revised one with the array init'r.. Which made the error go away, but the file didn't parse (all the records returned were full of default values, rather than parsed values)

Then I noticed a small problem with the schema in that the column names have leading spaces and the in-code schema definition doesn't. Wasn't aware that it mattered but when I adjusted the schema to have leading spaces, the "Body of.." error returned. I switched CompiledDataBinder to the typeof(void) version above, and it disappeared again, and now the file does also correctly parse. I also then reverted to the original code for CDB, and it also works...

So perhaps all in this bug report needs to be changed to more like "if one gets the schema wrong, then a bizarre red herring of an exception appears".

I think over the time, an good portion of the dev effort I've put in to getting Sylvan working with a new file has centered on error conditions or messages that were difficult to decipher, or where I couldn't get much useful debugging info. If there's a way to improve that it would be great, because I feel like I could eventually reach a stage where I'll swap out for a library that's lower performance purely because it's easier to use and has more comprehensive error messaging

[MarkPflug]

Sounds like you got it figured out?

The original issue is that there was nothing getting bound, because the headers had a space prefixed, which probably should have produced a binding error. Your code uses BindingMode.Any, which currently allows zero properties to be bound. Technically, it might make sense to allow a completely uninitialized T to be returned, since Any includes none, but I think in reality it would always mask a mistake of the user.

Would it have been more obvious if an UnboundMemberException had been thrown originally?

[ChairmanMawh]

Yeah, I've resolved to start off with bindingmode All in future, and then swap it to Any.. Even when I was debugging Sylvan's code today, I'm looking at a vis of the physicalColumns var and pulling my hair out wondering "why are there 54 entities here and every single one of them has no name at all.. infact, no anything at all - all the properties are the default values?" - it was so easy to miss the leading space in the data vs the schema

"Any" probably should have a special case for when nothing matches (to my mind Any excludes none, - I think along the same lines as LINQ where "".Any() is false. I understand why Any permits all cols/props to be unbound in the current impl though), and perhaps raise an error if a schema mentions a property that doesn't exist - while we can't reasonably control whether a column is or isn't in a CSV from one file to the next, the compiled code nominating A>B could raise an error if B doesn't exist as a proeprty in a compiled class.. MissingMemberException?

[ChairmanMawh]

Actually, thinking on it some more, I think my requirement to use Any is a consequence of not having available the mode that I really, actually want to use..

All notionally means "All columns in file related to All properties in model" but I have columns I don't want to use and for some files I have properties I cannot set (no data for them) but it doesn't matter for those props. I can't use AllColumns or AllProps either for the same reason; sometimes a file will have both unbound cols and result in unassigned props
.. and thus my only choice is to use Any, which doesn't check anything

Really, I'd like the schema, if I've used one, to be the driver for an All operation; if I've put columns in the schema that don't exist in the file, or i've mentioned props in the schema that don't exist in the class, then I should get an error so I can correct the code

In other words where we now have All/AllX driven by the file/class, have a different mode/setting where it's driven by the schema - do an All, but pretend that columns/props not mentioned in the schema are out of scope

[MarkPflug]

The data binder can use the DataMemberAttribute to allow mapping names that don't match the property. That attribute has an IsRequired member, but I don't currently look at it. I think it would make sense to enforce the binder binds all IsRequired properties regardless of BindingMode.

[MarkPflug]

I've made two changes.

1. the binder must bind at least one property regardless of binding mode or an UnboundMemberException will be thrown.
2. the binder now honors the DataMemberAttribute.IsRequired property, such that any properties with that flag must be bound, or an UnboundMemberException will be thrown.

These changes are included in this pre-release package:
https://www.nuget.org/packages/Sylvan.Data/0.2.13-b0001

I'll release a 0.2.13 final release at some point in the near future.