feat(map): Codegen for Rust Target

[jonathan-casey]

Description

Adds Rust code generation for Map Type.

Sample <String, String>

pub mut mock_map: HashMap<String, String> = HashMap::new();

Sample <Scalar String, Scalar>

pub mut mock_map: HashMap<String, String> = HashMap::new();

Sample <Scalar DateTime, Person>

pub mut mock_map: HashMap<DateTime<Utc>, Person> = HashMap::new();

Author Checklist

• Ensure you provide a DCO sign-off for your commits using the --signoff option of git commit.
• Vital features and changes captured in unit and/or integration tests
• Commits messages follow AP format
• Extend the documentation, if necessary
• Merging to main from fork:branchname

[coveralls]

coverage: 98.872% (-0.06%) from 98.928%
when pulling b098c94 on jonathan/codegen_map_rust
into 01bb536 on main.

[dselman]

@martinhalford can you review please?

[ekarademir]

Since none of the other generated fields a mutable by default, I think you can keep this immutable too. I didn't realise at first that this was forming a struct definition, I don't think you can use mut here anyway.

[ekarademir]

Also, I think this should be wrapped up in an option, and you are not initializing this in the struct definition, it's not valid Rust.

Suggested change

	parameters.fileWriter.writeLine(1, `pub mut ${this.toValidRustName(field.getName())}: HashMap<${rustKeyType}, ${rustValueType}> = HashMap::new();`);
	parameters.fileWriter.writeLine(1, `pub ${this.toValidRustName(field.getName())}: Option<HashMap<${rustKeyType}, ${rustValueType}>>,`);

[ekarademir]

toRustType definition is lacking Integer conversion, here

concerto-codegen/lib/codegen/fromcto/rust/rustvisitor.js

Lines 289 to 303 in 41428ed

	toRustType(type) {
	switch (type) {
	case 'DateTime':
	return 'DateTime<Utc>';
	case 'Boolean':
	return 'bool';
	case 'Long':
	return 'u64';
	case 'Double':
	return 'f64';
	default: {
	return type;
	}
	}
	}

Integer is one of the possibilities, here https://github.com/accordproject/concerto/blob/5b72c1951c4f1546cbc29a8a18e89d516efa74e3/packages/concerto-core/lib/introspect/scalardeclaration.js#L61-L75

I think we can convert Integer to u32.

[ekarademir]

Also checked DataTime<Utc> from the chronos library, it is hashable. So we are safe there.

[jonathan-casey]

JavaScript uses double-precision floating-point format, which is a 64-bit binary format defined by the IEEE 754-2008 standard. I think f64 is better suited?

[ekarademir]

Why would you map Integer to a floating point format?

[jonathan-casey]

It seems counter-intuitive, my line of thinking is there will be no loss of precision when working with JS numbers

[ekarademir]

I have two counterpoints for using f64. First: I would be very surprised if I see a float number when my concerto type is an integer. Especially the point of code generation is not JS compatibility.

More importantly, the second issue is that f64 does not implement hash. So it would break for keys.

https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=e6f21b9a4a19946b348a3ac81f93293a

[ekarademir]

I guess the 3rd one is that for Long we are converting them to u64, which should have been u128 anyway, but for integers it could at least be i64. I don't know, maybe @martinhalford can weight in.

[ekarademir]

Concerto documentation dictates that Integers should be i32 and longs i64 https://concerto.accordproject.org/docs/design/specification/model-properties#:~:text=Integer,64%20bit%20signed%20whole%20number. so existing conversion of Long is also wrong?

[ekarademir]

The comment on the scalars goes for primitive types too. https://github.com/accordproject/concerto/blob/5b72c1951c4f1546cbc29a8a18e89d516efa74e3/packages/concerto-core/lib/modelutil.js#L159-L162

[dselman]

@ ekarademir any chance you can get this PR over the line so we can merge?

[ekarademir]

@ ekarademir any chance you can get this PR over the line so we can merge?

Definitely!

[ekarademir]

@dselman #87 has been merged. I could only create a fork so I couldn't update this branch.

[martinhalford]

@martinhalford can you review please?

@dselman - apologies for the delayed response. Will do. Thanks @jonathan-casey for doing this.