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

Avoid use of Java Reflection (restructure part 5) #870

Merged
merged 61 commits into from
Apr 13, 2023

Conversation

aslesarenko
Copy link
Member

@aslesarenko aslesarenko commented Mar 7, 2023

This PR is focused on removing direct usages of Java reflection. Instead scalan.reflection package introduced in common module.
This is a next step towards Scala.js support.

The new reflection-like interface has two implementations:

  1. using Java reflection (see JRClass) - can be used in JVM and Ergo node (ensures it works the same doesn't break consensus)
  2. using static reflection (see SRClass) - uses special reflection metadata registrations (this can be used in JS)

Also:

  • a lot of code cleanup
  • missing ScalaDocs

@codecov
Copy link

codecov bot commented Mar 7, 2023

Codecov Report

Merging #870 (c68759c) into v5.0.6-RC (cf1f192) will increase coverage by 4.62%.
The diff coverage is 62.35%.

@@              Coverage Diff              @@
##           v5.0.6-RC     #870      +/-   ##
=============================================
+ Coverage      75.93%   80.56%   +4.62%     
=============================================
  Files            218      220       +2     
  Lines          13599    13997     +398     
  Branches         477      473       -4     
=============================================
+ Hits           10327    11276     +949     
+ Misses          3272     2721     -551     
Impacted Files Coverage Δ
common/src/main/scala/scalan/package.scala 100.00% <ø> (ø)
.../src/main/scala/scalan/reflection/StaticImpl.scala 0.00% <0.00%> (ø)
...on/src/main/scala/scalan/util/CollectionUtil.scala 82.24% <ø> (+0.96%) ⬆️
common/src/main/scala/scalan/util/StringUtil.scala 50.00% <ø> (+2.17%) ⬆️
...-lib/src/main/scala/special/collection/Colls.scala 84.61% <ø> (ø)
...ain/scala/special/collection/CollsOverArrays.scala 79.62% <ø> (ø)
...in/scala/special/collection/ExtensionMethods.scala 100.00% <ø> (ø)
...ib/src/main/scala/special/collection/Helpers.scala 100.00% <ø> (ø)
...ib/src/main/scala/special/collection/package.scala 80.00% <ø> (ø)
...re-lib/src/main/scala/special/sigma/SigmaDsl.scala 38.88% <ø> (ø)
... and 188 more

... and 15 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@aslesarenko aslesarenko marked this pull request as ready for review March 17, 2023 19:19
@aslesarenko aslesarenko requested a review from kushti March 22, 2023 12:53
override def getSimpleName: String = value.getSimpleName
override def getName: String = value.getName

/** A sequence that stores the constructors of this class. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just "Constructors of this class" ? see https://docs.scala-lang.org/style/scaladoc.html#general-style

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

constructors
}

/** Helper method that returns a sequence of `JRConstructor` objects that were at least
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"a sequence of" seems to be excessive here as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

* @param declaringClass The JRClass that declares this method.
* @param value The [[java.lang.reflect.Method]] instance that this JRMethod represents.
*/
class JRMethod private (declarigClass: JRClass[_], val value: Method) extends RMethod {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

declaringClass

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

}
}

/** Creates a new SRMethod instance with the given parameters and handler function.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But what does "a new SRMethod instance" mean? It is obvious from the function signature that the functions returns an RMethod instance, what scaladoc is saying above that ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarified

@@ -0,0 +1,117 @@
package scalan.reflection

import scala.collection.mutable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unused import

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

trait WOptionsDefs extends scalan.Scalan with WOptions {
self: WrappersModule =>

object WOption extends EntityObject("WOption") {
class WOptionCls extends EntityObject("WOption") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScalaDoc missed

Why WOptionCls , not WOptionImpl ?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because Impl is usually when some abstract class/interface is implemented. Here it is to:

  • avoid name collision
  • have a class type where WOption.type doesn't work (in reflection metadata, see usages of WOptionCls)


object WRType extends EntityObject("WRType") {
class WRTypeCls extends EntityObject("WRType") {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ScalaDoc missed

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

import scalan.reflection.RClass

// Abs -----------------------------------
trait WSpecialPredefsDefs extends scalan.Scalan with WSpecialPredefs {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

scaladocs missed in file

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file is just moved, has not been changed.

import special.collection.Coll
import special.sigma.{SigmaDslBuilder, AvlTree}

/** Reflection metadata for `interpreter` module. */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, this scaladoc does not say much about what is going on here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed

class ZLastSpec extends AnyPropSpec with BeforeAndAfterAll {

property("the last property") {

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

???

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

clarified

@aslesarenko aslesarenko requested a review from kushti April 11, 2023 16:51
@aslesarenko aslesarenko changed the base branch from v5.0.6-RC to v5.0.7-RC April 13, 2023 15:05
@aslesarenko aslesarenko merged commit cd9cf86 into v5.0.7-RC Apr 13, 2023
@aslesarenko aslesarenko deleted the restructure-part5 branch April 13, 2023 15:08
@aslesarenko aslesarenko mentioned this pull request Aug 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants