Scala3 migration2 #122

NicolasRouquette · 2022-05-15T23:04:43Z

Thanks to the Scala3 migration tool, the migration was very easy and a fun exercise!

There are 2 compiler warnings that should be fixed in the source code:

src/main/scala/analysis/Semantics/TypeVisitor.scala


  final def matchType(in: In, t: Type): Out =
    t match {
      case t : Type.AbsType => absType(in, t)
      case t : Type.AnonArray => anonArray(in, t)
      case t : Type.AnonStruct => anonStruct(in, t)
      case t : Type.Array => array(in, t)
      case Type.Boolean => boolean(in)
      case t : Type.Enum => enumeration(in, t)
      case t : Type.Float => float(in, t)
      case Type.Integer => integer(in)
      case t : Type.PrimitiveInt => primitiveInt(in, t)
      case t : Type.String => string(in, t)
      case t : Type.Struct => struct(in, t)
      case _ => default(in, t) // compiler detects this is unreachable unless t is null. 
    }

src/main/scala/analysis/Semantics/ValueVisitor.scala

  final def matchValue(in: In, v: Value): Out = {
    v match {
      case v : Value.PrimitiveInt => primitiveInt(in, v)
      case v : Value.Integer => integer(in, v)
      case v : Value.Float => float(in, v)
      case v : Value.Boolean => boolean(in, v)
      case v : Value.String => string(in, v)
      case v : Value.AnonArray => anonArray(in, v)
      case v : Value.AbsType => absType(in, v)
      case v : Value.Array => array(in, v)
      case v : Value.EnumConstant => enumConstant(in, v)
      case v : Value.AnonStruct => anonStruct(in, v)
      case v : Value.Struct => struct(in, v)
      case _ => default(in, v) // compiler detects this is unreachable unless v is null. 
    }
  }

Please advise w.r.t how to proceed to eliminate the warning.

Fixed type annotations on toString methods.

- Changed Binop from private to protected[analysis] and all 'binop' methods - Added explicit type annotations on specializations of Value.getType that return a subtype of Type Compiles/tests cleanly with scala 3.1 (thanks to the migrate tool).

[warn] -- [E121] Pattern Match Warning: C:\opt\local\github.fprime\fpp\compiler\lib\src\main\scala\analysis\Semantics\TypeVisitor.scala:45:11 [warn] 45 | case _ => default(in, t) [warn] | ^ [warn] |Unreachable case except for null (if this is intentional, consider writing case null => instead). [warn] -- [E121] Pattern Match Warning: C:\opt\local\github.fprime\fpp\compiler\lib\src\main\scala\analysis\Semantics\ValueVisitor.scala:45:11 [warn] 45 | case _ => default(in, v) [warn] | ^ [warn] |Unreachable case except for null (if this is intentional, consider writing case null => instead). All tests pass.

bocchino · 2022-05-15T23:32:58Z

Thanks for doing this!

Please advise w.r.t how to proceed to eliminate the warning.

If the compiler warns that those cases are unreachable, then let's delete them.

Looking over the changes, I see that most of them add return type annotations. Some of them remove the final keyword.

With regard to the return type annotations, are these changes required or recommended?
With regard to eliminating final, what is going on? Are the declarations now final by default? I would prefer to keep them final if possible.

bocchino · 2022-05-15T23:34:30Z

Also, in cases where you added .apply, what is going on? Is this a new way to pass a function, instead of writing f _?

bocchino · 2022-05-15T23:35:50Z

With regard to the return type annotations, are these changes required or recommended?

Overall the code is more verbose, but also more explicit about the return type.

bocchino · 2022-05-15T23:51:04Z

I found this in the Scala 3 Migration Guide:

It is always good practice to write the result types of all public values and methods explicitly. It prevents the public API of your library from changing with the Scala version, because of different inferred types.

https://docs.scala-lang.org/scala3/guides/migration/incompat-type-inference.html

NicolasRouquette · 2022-05-16T00:00:05Z

Here are some comments about the changes that I had to do above and beyond what the scala3 migration tool did.

Regarding final, with Scala3, the following code produces a E147 warning:

      sealed trait Type
      final case object Character extends Type

[warn] -- [E147] Syntax Warning: C:\opt\local\github.fprime\fpp\compiler\lib\src\main\scala\analysis\Semantics\Format.scala:34:6
[warn] 34 |      final case object Character extends Type
[warn]    |      ^^^^^
[warn]    |      Modifier final is redundant for this definition

To avoid getting lots of E147 warning noise, I deleted all these redundant keywords.

Regarding the return type annotations

There was a subtle Scala 2.13 vs. Scala 3 difference w.r.t analysis.Value.getType: Type where Value subtypes return subtypes of Type but would do so without explicitly declaring the return type, e.g:

  case class Struct(anonStruct: AnonStruct, t: Type.Struct) extends Value {
 ...
    override def getType = t // the return type is Type.Struct, not Type!

With Scala3, the compiler detected an error in ValueXmlWriter:

    override def struct(s: XmlWriterState, v: Value.Struct): String = {
      val structType = v.getType // Here, the compiler does not know that v.getType will be Type.Struct !
      val data = structType.node._2.data // Unless structType is known to be a Type.Struct, this code does not compile!

Regarding apply, the Scala3 compiler marked the usage as deprecated unless we explicitly refer to .apply.

Overall, the migration tool is really impressive! It did most of the changes for me, the only things were the corner cases described above.

Added -Xfatal-warnings. All tests pass

bocchino · 2022-05-16T00:07:45Z

compiler/lib/src/main/scala/codegen/XmlWriter/ComputeXmlFiles.scala

    val (_, node1, _) = node
    val data = node1.data
    visitList(s, data.members, matchModuleMember)
  }

-  override def defPortAnnotatedNode(s: State, node: Ast.Annotated[AstNode[Ast.DefPort]]) = {
+  override def defPortAnnotatedNode(s: State, node: Ast.Annotated[AstNode[Ast.DefPort]]): Either[CodeGenError.DuplicateXmlFile,XmlWriterState] = {


I'm wondering about this change and others like it. It looks like the migration tool has inferred a specific type and added it as the return type. But this coding style seems like a pain to write by hand. The style I was using before is

Make a type alias for the more general type, e.g,. type Result = Either[Error,State]

Avoid writing the return type at all where possible

Otherwise use the type alias

In the new style, it seems to use more specific and more complex instantiations of parameterized types. Is this just an artifact of the migration, or will Scala 3 require this more specific type annotation going forward? And will we have to write these types by hand? Again, this seems like it could be a pain.

Completely agree with you... Let me see if I can use the type alias instead.

OK, sounds good. If Scala 3 is just being more fussy about writing an explicit return type, maybe we can copy over the return type from the method being overridden? Except in rare cases like the one you identified with the structType, I think this is what is intended and should work.

Regarding type aliases, why are there two different aliases for the same type?

trait AstStateVisitor extends AstVisitor { type State type In = State type Out = Result.Result[State] type Result = Result.Result[State]

FYI: I think it would be preferable to avoid synonym aliases because it makes the code harder to read.
e.g:

AstVisitor has:

def transUnit(in: In, tu: Ast.TransUnit): Out

and ComputeXmlFiles, a subtype of AstVisitor has:

override def transUnit(s: State, tu: Ast.TransUnit): Result

Since Out and Result are synonyms for the same type, the compiler is OK; however, it creates unnecessary confusion.

Regarding eliminating the return types, unfortunately, the presence of type aliases is potentially confusing.
Is Either[CodeGenError.DuplicateXmlFile, XmlWriterState] the appropriate return type or should it be Out or Result?
All 3 are acceptable to the compiler; for a human, I think the intended return type would help with code legibility, particularly when there is only 1 sensible type alias -- e.g. Out or Result but not both!

FYI:

The IDE find 24 usages of AstStateVisitor.Out and 113 usages of AstStateVisitor.Result.
I propose refactoring all 24 usages of AstStateVisitor.Out as AstStateVisitor.Result.

@bocchino Is this OK with you?

The rationale for having both is that an AstVisitor is a generic operation that take an In to an Out, whereas an AstStateVisitor manages state. Also, a Result is the result of an operation that can return an error.

So I'd prefer to keep In/Out for AstVisitor and State/Result for AstStateVisitor.

Refactoring uses of AstStateVisitor.Out to AstStateVisitor.Result seems good. We still need to keep the type alias AstStateVisitor.Out to satisfy the inheritance from AstVisitor. Overall I think having synonyms for the same type reflecting its use in different contexts is OK.

In English, the logic is something like this: "AstVisitor takes an In to an Out, so to inherit from it you have to define an In type and an Out type. AstStateVisitor takes a State to a Result, so it defines those types. It also inherits from AstVisitor, and its State type is the In type of AstVisitor, and its Result type is the Out type of AstVisitor. So we create those type aliases to satisfy the inheritance contract with AstVisitor."

NicolasRouquette · 2022-05-16T00:08:00Z

@bocchino Are your concerns properly addressed?

bocchino · 2022-05-16T00:08:58Z

Getting there. See the comment I just left. I want to understand why these more complicated and specific type annotations have been added, and whether we need to keep writing them going forward.

bocchino · 2022-05-16T00:10:01Z

Overall, I am concerned about having to write explicit Either[ < complex parameter instantiation > ] types in the code. I never had to do that before.

NicolasRouquette · 2022-05-16T03:17:35Z

I believe the migration tool should have not added explicit return types for overrides methods as long as the calculated return type matches the super method's return type. The return type is required when the calculated return type of the overridden method is a subtype of the super method. I am working on removing these cases and I will file a bug report on the migration tool.

bocchino · 2022-05-16T03:33:16Z

I believe the migration tool should have not added explicit return types for overrides methods as long as the calculated return type matches the super method's return type.

I think the tool may be enforcing the recommendation in the migration notes, to put in return types explicitly for public methods. I think that's OK. I think the case to watch out for is

The overridden method has type T1.
The body of the overriding method has type T2 <: T1, and the overriding method has no explicit return type. In this case, Scala 2.13 infers T2 as the return type, and Scala 3 infers T1 as the return type.
The return type we actually want for the overriding method is T1. (In other words, if we had written an explicit return type in the Scala 2.13 version, we would have written T1, not T2.)

In this case the migration tool conservatively assumes that we want to preserve the type inferred by Scala 2.13, so it inserts T2 as the return type. I think we should manually change the type to T1. (Or we could delete the type and let Scala 3 infer T1, but this seems contrary to the recommendation in the migration notes.)

All tests pass

NicolasRouquette · 2022-05-16T05:01:48Z

@bocchino Can you please review the changes?

I agree with your analysis.

I carefully reviewed most of the cases where the migration tool added a return type to an overriding method and removed a lot of such type annotations if possible, leaving those that are strictly required.

There were a few methods that lacked the override keyword; I added the keyword and removed the type annotation where possible.

I generally left the type annotations that the migration tool added on non-overriding methods as this helps improve code legibility.

bocchino · 2022-05-16T15:43:30Z

compiler/lib/src/main/scala/analysis/Analyzers/TypeExpressionAnalyzer.scala

@@ -42,9 +42,9 @@ trait TypeExpressionAnalyzer
    } yield a
  }

-  def typeNameNode(a: Analysis, node: AstNode[Ast.TypeName]) = matchTypeNameNode(a, node)
+  def typeNameNode(a: Analysis, node: AstNode[Ast.TypeName]): Out = matchTypeNameNode(a, node)


Should this be Result instead of Out?

I replaced it.

Note that the confusion comes from here:

trait AstStateVisitor extends AstVisitor { type State type In = State type Out = Result.Result[State] type Result = Result.Result[State] ...

Generally, I prefer avoiding type synonyms like Out and Result because one can easily get confused with inconsistent usage as was the case here.

compiler/lib/src/main/scala/analysis/Semantics/TypeVisitor.scala

bocchino · 2022-05-16T15:44:45Z

compiler/lib/src/main/scala/analysis/Semantics/Value.scala

@@ -34,7 +35,7 @@ sealed trait Value {
  }

  /** Generic binary operation */
-  def binop(op: Value.Binop)(v: Value): Option[Value] = None
+  protected[analysis] def binop(op: Value.Binop)(v: Value): Option[Value] = None


What is the rationale for replacing private with protected[analysis]?

I replaced it with private[analysis].

Without scoping to the analysis package, this code would not compile:

case class EnumConstant(value: (Name.Unqualified, BigInt), t: Type.Enum) extends Value { override private[analysis] def binop(op: Binop)(v: Value) = convertToRepType.binop(op)(v)

The reason is that convertToRepType is a PrimitiveInt, not an EnumConstant' so binop` would not be accessible.

compiler/lib/src/main/scala/analysis/Semantics/ValueVisitor.scala

bocchino · 2022-05-16T15:45:20Z

compiler/lib/src/main/scala/ast/AstStateVisitor.scala

@@ -14,7 +14,7 @@ trait AstStateVisitor extends AstVisitor {
  type Result = Result.Result[State]

  /** Default state transformation */
-  def default(s: State) = Right(s)
+  def default(s: State): Out = Right(s)


Should this be Result?

This is an override method so I removed the type annotation.

bocchino · 2022-05-16T15:45:42Z

compiler/lib/src/main/scala/codegen/ComputeGeneratedFiles.scala

@@ -15,13 +15,13 @@ object ComputeGeneratedFiles {
    } 
    yield xmlFiles ++ cppFiles

-  def getCppFiles(tul: List[Ast.TransUnit]) =
+  def getCppFiles(tul: List[Ast.TransUnit]): Either[Error,List[String]] =


Can we use Result.Result instead of Either[...] here? Similarly for the other uses of Either.

bocchino

Looks great! The code is much more readable now.

I ran the tests and they all passed. Just a few more comments.

bocchino · 2022-05-16T16:00:40Z

Can you also please apply the following patch to your branch:

diff --git a/compiler/install b/compiler/install
index 22f35e32..88e27221 100755
--- a/compiler/install
+++ b/compiler/install
@@ -7,6 +7,8 @@
 # FPP_SBT_FLAGS
 # ----------------------------------------------------------------------
 
+scala_version="3.1.2"
+
 if test $# -gt 1
 then
   echo 'usage: install dest-dir' 1>&2
@@ -61,7 +63,8 @@ mkdir -p $dest
 echo "Installing tools at $dest"
 for tool in $tools
 do
-  jar=`find tools/$tool -name "*$name*assembly*.jar"`
+  jar=`find tools/$tool -name "*$name*assembly*.jar" | grep "target/scala-$scala_version"`
+  echo "  $jar"
   cp $jar $dest/$tool.jar
   echo '#!/bin/sh

I noticed that if the 2.13 and 3.1.2 binaries were both in there, the installer was not able to select the correct ones and failed.

NicolasRouquette · 2022-05-16T21:42:54Z

I cleaned up the Either[Error,T] types as you suggested.

Regarding Graal VM, I think we should ask the scala native folks if they have developed a github action workflow to build this as part of CI.

bocchino · 2022-05-16T22:38:57Z

Looks good. Unfortunately updating GraalVM did not help. Also running the tracing agent as discussed here

https://docs.oracle.com/en/graalvm/enterprise/19/guide/reference/native-image/tracing-agent.html

did not help. I tried that because of comments here

oracle/graal#2694

suggesting it could remedy the following runtime error when compiling with Scala 3 and then native-image:

Exception in thread "main" scala.runtime.LazyVals$$anon$1
	at scala.runtime.LazyVals$.$init$$$anonfun$3(LazyVals.scala:18)
	at scala.Option.getOrElse(Option.scala:201)
	at scala.runtime.LazyVals$.<clinit>(LazyVals.scala:19)
	at scopt.OParser.<clinit>(OParser.scala:259)
	at scopt.OParser$.apply(OParser.scala:260)
	at scopt.OParserBuilder.wrap(OParser.scala:79)
	at scopt.OParserBuilder.programName(OParser.scala:7)
	at fpp.compiler.FPPSyntax$.<clinit>(fpp-syntax.scala:77)
	at fpp.compiler.FPPSyntax.main(fpp-syntax.scala)

NicolasRouquette · 2022-05-16T22:45:06Z

Can you put in a README somewhere the procedure you've followed to build the graal native image?
I would like to try this...

bocchino · 2022-05-16T23:01:37Z

Can you put in a README somewhere the procedure you've followed to build the graal native image?
I would like to try this...

Yes, I'm working on it. I'll push that change to main.

bocchino · 2022-05-17T00:43:12Z

OK, I added a section on building native binaries to the README in the compiler directory:

https://github.com/fprime-community/fpp/tree/main/compiler#building-native-binaries

bocchino · 2022-05-17T01:17:38Z

I pushed a change that simplifies the procedure.

NicolasRouquette · 2022-05-17T04:43:04Z

I think we need representative runs of the JVM-based application running with -agentlib:native-image-agent to generate appropriate configuration files as described here: https://gchudnov.github.io/blog/scala-graalvm/

jni-config.json
reflect-config.json
proxy-config.json
resource-config.json

Using the install and release scripts from the main branch (I rebased this branch on main), I get this:

./native-fpp-Linux-x86_64/fpp-check -help
Exception in thread "main" java.lang.ExceptionInInitializerError
        at scala.runtime.Statics$VM.mkHandle(Statics.java:167)
        at scala.runtime.Statics$VM.<clinit>(Statics.java:155)
        at com.oracle.svm.core.classinitialization.ClassInitializationInfo.invokeClassInitializer(ClassInitializationInfo.java:375)
        at com.oracle.svm.core.classinitialization.ClassInitializationInfo.initialize(ClassInitializationInfo.java:295)
        at scala.runtime.Statics.releaseFence(Statics.java:148)
        at scopt.Read$.<clinit>(Read.scala:130)
        at com.oracle.svm.core.classinitialization.ClassInitializationInfo.invokeClassInitializer(ClassInitializationInfo.java:375)
        at com.oracle.svm.core.classinitialization.ClassInitializationInfo.initialize(ClassInitializationInfo.java:295)
        at scopt.OParserBuilder.programName(OParser.scala:7)
        at fpp.compiler.tools.FPPCheck$.<clinit>(fpp-check.scala:75)
        at com.oracle.svm.core.classinitialization.ClassInitializationInfo.invokeClassInitializer(ClassInitializationInfo.java:375)
        at com.oracle.svm.core.classinitialization.ClassInitializationInfo.initialize(ClassInitializationInfo.java:295)
        at fpp.compiler.tools.FPPCheck.main(fpp-check.scala)
        Suppressed: java.lang.NoSuchMethodException: no such method: java.lang.invoke.VarHandle.releaseFence()void/invokeStatic
                at java.lang.invoke.MemberName.makeAccessException(MemberName.java:963)
                at java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1101)
                at java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:2030)
                at java.lang.invoke.MethodHandles$Lookup.findStatic(MethodHandles.java:1102)
                at scala.runtime.Statics$VM.mkHandle(Statics.java:161)
                ... 12 more
        Caused by: java.lang.NoSuchMethodError: java.lang.invoke.VarHandle.releaseFence()
                at java.lang.invoke.MethodHandleNatives.resolve(MethodHandleNatives.java:230)
                at java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1070)
                at java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1098)
                ... 15 more
Caused by: java.lang.NoSuchMethodException: no such method: sun.misc.Unsafe.storeFence()void/invokeVirtual
        at java.lang.invoke.MemberName.makeAccessException(MemberName.java:963)
        at java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1101)
        at java.lang.invoke.MethodHandles$Lookup.resolveOrFail(MethodHandles.java:2030)
        at java.lang.invoke.MethodHandles$Lookup.findVirtual(MethodHandles.java:1194)
        at scala.runtime.Statics$VM.mkHandle(Statics.java:165)
        ... 12 more
Caused by: java.lang.NoSuchMethodError: sun.misc.Unsafe.storeFence()
        at java.lang.invoke.MethodHandleNatives.resolve(MethodHandleNatives.java:230)
        at java.lang.invoke.MemberName$Factory.resolve(MemberName.java:1070)
        at java.lang.invoke.MemberName$Factory.resolveOrFail(MemberName.java:1098)

After running the JVM-based fpp-check.jar w/ the -agentlib:native-image-agent option, the reflect-config.json has these entries:

[
{
  "name":"java.lang.ClassValue"
},
{
  "name":"java.lang.String[]"
},
{
  "name":"java.lang.invoke.VarHandle",
  "methods":[{"name":"releaseFence","parameterTypes":[] }]
},
{
  "name":"scopt.OParser$",
  "fields":[{"name":"0bitmap$1", "allowUnsafeAccess":true}]
},
{
  "name":"sun.misc.Unsafe",
  "allDeclaredFields":true
}
]

Moving these files to the resource folder (tools/fpp-check/src/resources/META-INF/...), I rebuilt the native app.

This time, it worked like a charm!

./fpp-check -h 
fpp-check [unknown version]
Usage: fpp-check [options] [file ...]

  -h, --help               print this message and exit
  -u, --unconnected <file>
                           write unconnected ports to file
  file ...                 input files

This is the executable:

file fpp-check  
fpp-check: ELF 64-bit LSB shared object, x86-64, version 1 (SYSV), dynamically linked, interpreter /lib64/ld-linux-x86-64.so.2, BuildID[sha1]=e66a3d24ea933261dbcd6cc8c5d330f1f467c929, for GNU/Linux 3.2.0, with debug_info, not stripped

Also, I see that the native packager provides some support for building native binaries:
https://sbt-native-packager.readthedocs.io/en/latest/formats/graalvm-native-image.html

The question here is can you suggest a representative set of JVM-based runs we can do w/ the agent to generate the appropriate config for the native runs to work without problems?

bocchino · 2022-05-17T18:10:00Z

This is great, thanks!

can you suggest a representative set of JVM-based runs we can do w/ the agent to generate the appropriate config for the native runs to work without problems

It seems the problems occur in the interaction with scopt, which occurs at the beginning (parsing command-line arguments). So it seems like just running one of the applications, even with no input, should be enough.

bocchino · 2022-05-17T18:10:49Z

I think when I tried this I created the JSON files but didn't copy them into the right place.

NicolasRouquette · 2022-05-18T00:59:09Z

The above commit adds a new folder: lib/src/main/resources/META-INF/native-image with 5 JSON configuration files generated by the native-image-agent.

Using just clean;install;release, I was able to execute all native images with -h successfully.

bocchino · 2022-05-18T15:24:04Z

This is great progress! However the JVM / GraalVM interaction is still having issues.

I confirmed that the native binaries work with -h
I ran the unit tests by copying the native binaries into bin and running ./test. Many failed with Java exceptions. The ones I examined were complaining about missing fields during initialization. They seem to have to do with the Parser library.
I re-ran the trace analysis with an input file. As expected, the JSON files have more entries in them. This result makes sense, because when we run with -h we are not exercising the parser.
However, I still could not get the native binaries to work as expected. For example, I saw this:
a. Build and install JAR files with ./install.
b. Run fpp-syntax.jar on this file with trace analysis and put the results in the META-INF/native-image directory. As expected, there were new entries in the JSON files.
c. Generate the native binary for fpp-syntax with ./release, picking up the updated JSON files.
d. Run the native binary for fpp-syntax on the FPP file referred to in (b). It still failed with a runtime exception.

Item (4) is a bit of a stumper. Either there is something wrong with my procedure, or the trace analysis is not working in this case, since the same input that produced the trace is causing a failed run.

…t-dir=<path to lib/resources/META-INF/native-image>; release; ./native-fpp-Linux-x86_64/fpp-syntax ./tools/fpp-syntax/test/syntax.fpp

NicolasRouquette · 2022-05-18T16:18:13Z

I followed your procedure, which resulted in updated JSON config files (see commit above).

./native-fpp-Linux-x86_64/fpp-syntax ./tools/fpp-syntax/test/syntax.fpp

No errors.

NicolasRouquette · 2022-05-18T16:19:18Z

Hum, can you check this update? #122 (comment) * Nicolas. From: Rob Bocchino ***@***.***> Sent: Wednesday, May 18, 2022 8:24 AM To: fprime-community/fpp ***@***.***> Cc: Rouquette, Nicolas F (US 319K) ***@***.***>; Author ***@***.***> Subject: [EXTERNAL] Re: [fprime-community/fpp] Scala3 migration2 (PR #122) This is great progress! However the JVM / GraalVM interaction is still having issues. 1. I confirmed that the native binaries work with -h 2. I ran the unit tests by copying the native binaries into bin and running ./test. Many failed with Java exceptions. The ones I examined were complaining about missing fields during initialization. They seem to have to do with the Parser library. 3. I re-ran the trace analysis with some input file. As expected, the JSON files have more entries in them. This result makes sense, because when we run with -h we are not exercising the parser. 4. However, I still could not get the native binaries to work as expected. For example, I saw this: a. Build and install JAR files with ./install. b. Run fpp-syntax.jar on this file<https://urldefense.us/v3/__https:/github.com/fprime-community/fpp/blob/main/compiler/tools/fpp-syntax/test/syntax.fpp__;!!PvBDto6Hs4WbVuu7!YyijdlzF4nlljhawbYuaJWnZnadcsa0zm80-OtsFaRG_-zCQtsaoBxeaWhge-pOSu7GtHBoeO0zKoA$> with trace analysis and put the results in the META-INF/native-image directory. As expected, there were new entries in the JSON files. c. Generate the native binary for fpp-syntax with ./release, picking up the updated JSON files. d. Run the native binary for fpp-syntax on the FPP file referred to in (b). It still failed with a runtime exception. Item (4) is a bit of a stumper. Either there is something wrong with my procedure, or the trace analysis is not working in this case, since the same input that produced the trace is causing a failed run. — Reply to this email directly, view it on GitHub<https://urldefense.us/v3/__https:/github.com/fprime-community/fpp/pull/122*issuecomment-1130160298__;Iw!!PvBDto6Hs4WbVuu7!YyijdlzF4nlljhawbYuaJWnZnadcsa0zm80-OtsFaRG_-zCQtsaoBxeaWhge-pOSu7GtHBrbciUJOg$>, or unsubscribe<https://urldefense.us/v3/__https:/github.com/notifications/unsubscribe-auth/AAR767VQZK3ECQBPZQQVNJTVKUDSHANCNFSM5V7YQK7Q__;!!PvBDto6Hs4WbVuu7!YyijdlzF4nlljhawbYuaJWnZnadcsa0zm80-OtsFaRG_-zCQtsaoBxeaWhge-pOSu7GtHBpY8XQh1A$>. You are receiving this because you authored the thread.Message ID: ***@***.******@***.***>>

bocchino · 2022-05-18T18:16:03Z

Great, I confirmed that all the fpp-syntax tests pass now. Can you do the following:

Please merge this PR into your branch. It includes some updates to the release install script and to the release documentation. It should also fix the merge conflict noted below.
On your branch, after merging the PR, please update the release documentation here with documentation on how to run the tracing tool to generate the JSON files. There is obviously something you are doing right that I am doing wrong.
We need to run some more traces and update the JSON files. Let's try these:
a. fpp-check on this file, to pick up format parsing.
b. fpp-from-xml on this file to pick up XML parsing.

I think we will have to use the config-merge-dir option as discussed here to merge the traces generated in steps 3.a and 3.b. The merge of those traces should cover all the errors we've seen so far. Getting there...

bocchino · 2022-05-18T18:20:48Z

Note: We are done with GraalVM issues when we can do this:

% ./release
% cp native-fpp-[platform]/* bin
% ./test

and all the unit tests pass, except for one known failure in fpp-from-xml because the XML parsing library doesn't produce correct error messages when compiled natively.

Revert changes to parameter types

NicolasRouquette · 2022-05-20T01:05:30Z

Since GraalVM provides a tracing agent option for collecting configuration data across multiple runs, let's use this mechanism to collect tracing configuration data for the entire unit test suite! Since the unit test suite provides great code coverage, the tracing configuration should be reasonably complete.

Collect configuration data
- Generate the application scripts in the bin folder w/ the option: -agentlib:native-image-agent=config-merge-dir=....
- ./test
Build native images
- ./release
Run the tests using the native images
- cp native-fpp-[platform]/* bin
- ./test

For step 1, I modified the install script to generate the appropriate tracing agent option.

I got 4 error from the first step:

[ tools/fpp-to-xml/test/port/run ]
port_kwd_name                                               PASSED
port_ok                                                     ../scripts/run.sh: 1: fpp-depend: not found
FAILED

Is this due to a relative path for fpp-depend here:
https://github.com/NicolasRouquette/fpp/blob/scala3-migration2/compiler/tools/fpp-to-xml/test/port/run.sh#L13

[ tools/fpp-format/test/run ]
component                                                   PASSED
escaped_strings                                             PASSED
include                                                     ./run: 43: fpp-syntax: not found
FAILED
kwd_names                                                   ./run: 49: fpp-syntax: not found
FAILED
no_include                                                  ./run: 55: fpp-syntax: not found
FAILED

These are due to problems in this script:

https://github.com/NicolasRouquette/fpp/blob/scala3-migration2/compiler/tools/fpp-format/test/run#L43

I fixed all 4 errors in the scripts and made sure that all tests execute properly.

After steps 2 and 3, all unit tests pass OK!

The performance difference between running these tests in the JVM vs. native applications is quite impressive!

JVM:

All tests PASSED
./test  851.08s user 315.94s system 208% cpu 9:20.48 total

Native:

All tests PASSED
./test  27.91s user 5.78s system 253% cpu 13.275 total

These tests were done on an AMD Ryzen Threadripper PRO 3955WX 16-Cores, 3892 MHz in WSL2 from Windows 10.

bocchino · 2022-05-20T01:38:31Z

Super, this is great news!! I confirmed that all the tests pass with the GraalVM images on my MacBook Pro.

Can you make one small change: Please comment this line out (leaving it in as a comment):

https://github.com/NicolasRouquette/fpp/blob/2f5ce04ddf6df9f000af9334d91471c77514452b/compiler/install#L74

Please then add the same line, but without calling the tracing agent. That way we don't require the tracing agent to run the tools. I'll separately add a script with an option to run the tracing agent.

bocchino · 2022-05-23T18:01:36Z

Can you make one small change

I made this change myself, to resolve the conflict.

NicolasRouquette · 2022-05-23T18:42:46Z

Thanks

NicolasRouquette added 3 commits May 15, 2022 15:01

Applied 'migrate-syntax lib'

3e00b42

Fixed type annotations on toString methods.

Fixes after 'migrate lib'

e3fd73a

- Changed Binop from private to protected[analysis] and all 'binop' methods - Added explicit type annotations on specializations of Value.getType that return a subtype of Type Compiles/tests cleanly with scala 3.1 (thanks to the migrate tool).

Removed unreachable cases.

aba3a19

Added -Xfatal-warnings. All tests pass

bocchino reviewed May 16, 2022

View reviewed changes

Removed superfluous method override return types.

49fc431

All tests pass

NicolasRouquette force-pushed the scala3-migration2 branch from 7293985 to 49fc431 Compare May 16, 2022 04:55

bocchino reviewed May 16, 2022

View reviewed changes

compiler/lib/src/main/scala/analysis/Semantics/TypeVisitor.scala Show resolved Hide resolved

bocchino reviewed May 16, 2022

View reviewed changes

compiler/lib/src/main/scala/analysis/Semantics/ValueVisitor.scala Show resolved Hide resolved

bocchino reviewed May 16, 2022

View reviewed changes

NicolasRouquette added 4 commits May 16, 2022 10:02

updated install script

ed15ee1

Removed more override methods return types.

515f379

Finished removing superfluous override method return types.

6c9e324

Minor changes per @bocchino's review

132207c

Revert changes to parameter types

84c7698

bocchino mentioned this pull request May 17, 2022

Migrate to Scala 3 #123

Closed

2 tasks

bocchino and others added 2 commits May 17, 2022 13:52

Merge branch 'main' into scala3-migration

dfc12cf

native-image-agent instrumentation files

bff233b

Updated bin/fpp-syntax with -agentlib:native-image-agent=config-outpu…

24209c5

…t-dir=<path to lib/resources/META-INF/native-image>; release; ./native-fpp-Linux-x86_64/fpp-syntax ./tools/fpp-syntax/test/syntax.fpp

NicolasRouquette added 2 commits May 18, 2022 12:43

Merge pull request #1 from fprime-community/scala3-migration

374892f

Revert changes to parameter types

native configuration passes all tests

2f5ce04

Merge branch 'main' into scala3-migration2

1afc928

bocchino mentioned this pull request May 23, 2022

Scala 3 migration #126

Merged

bocchino merged commit 4243726 into nasa:main May 23, 2022

bocchino mentioned this pull request May 27, 2022

Scala3 migration #120

Closed

Scala3 migration2 #122

Scala3 migration2 #122

Conversation

NicolasRouquette commented May 15, 2022

bocchino commented May 15, 2022

bocchino commented May 15, 2022

bocchino commented May 15, 2022

bocchino commented May 15, 2022

NicolasRouquette commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocchino May 16, 2022 • edited Loading

Choose a reason for hiding this comment

NicolasRouquette commented May 16, 2022

bocchino commented May 16, 2022

bocchino commented May 16, 2022

NicolasRouquette commented May 16, 2022

bocchino commented May 16, 2022

NicolasRouquette commented May 16, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bocchino left a comment

Choose a reason for hiding this comment

bocchino commented May 16, 2022

NicolasRouquette commented May 16, 2022

bocchino commented May 16, 2022

NicolasRouquette commented May 16, 2022

bocchino commented May 16, 2022

bocchino commented May 17, 2022

bocchino commented May 17, 2022

NicolasRouquette commented May 17, 2022

bocchino commented May 17, 2022

bocchino commented May 17, 2022 • edited Loading

NicolasRouquette commented May 18, 2022

bocchino commented May 18, 2022 • edited Loading

NicolasRouquette commented May 18, 2022

NicolasRouquette commented May 18, 2022 via email

bocchino commented May 18, 2022 • edited Loading

bocchino commented May 18, 2022 • edited Loading

NicolasRouquette commented May 20, 2022

bocchino commented May 20, 2022

bocchino commented May 23, 2022

NicolasRouquette commented May 23, 2022

bocchino May 16, 2022 •

edited

Loading

bocchino commented May 17, 2022 •

edited

Loading

bocchino commented May 18, 2022 •

edited

Loading

bocchino commented May 18, 2022 •

edited

Loading

bocchino commented May 18, 2022 •

edited

Loading