Closed MF42-DZH closed 8 months ago
Critical roadblock at the moment is the usage of WeakHashMap
to prevent memory leaks for flatMap
-based parsers, as they are not available in ScalaJS (and possibly not in Scala Native either).
Added XWeakMap
for platform-dependent unified weak hash map implementation, but the JS implementation is hand-rolled, and may contain mistakes. WeakMap
from ECMA 6 would be nice to have instead of this hand-rolled implementation.
XWeakMap
to reduce code duplication.
XCollector
implementations too.XWeakMap
.XWeakMap
for JS is undergoing testing and fixes in a different branch. Cannot review this PR until MF42-DZH:xweakmap-tests
is merged into MF42-DZH:dev
.
Note: also waiting on typelevel/sbt-typelevel#659 in order to merge some source files together for less redundancy.
Other than the need to merge some sources together for Scala 2.13 and 3 (but that is not possible at the moment), these changes should be ready for review.
Code Climate has analyzed commit 2539d6e1 and detected 12 issues on this pull request.
Here's the issue category breakdown:
Category | Count |
---|---|
Complexity | 3 |
Duplication | 4 |
Style | 5 |
The test coverage on the diff in this pull request is 99.2% (50% is the threshold).
This pull request will bring the total coverage in the repository to 93.3% (0.8% change).
View more on Code Climate.
Something doesn't seem right with what running prePR
did to ci.yml
on dd2dc17:
@@ -123,11 +123,11 @@ jobs:
- name: Make target directories
if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master')
- run: mkdir -p parsley/native/target parsley/jvm/target parsley/js/target project/target
+ run: mkdir -p parsley-debug/jvm/target parsley-debug/native/target parsley/native/target parsley/jvm/target parsley-debug/js/target parsley/js/target project/target
- name: Compress target directories
if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master')
- run: tar cf targets.tar parsley/native/target parsley/jvm/target parsley/js/target project/target
+ run: tar cf targets.tar parsley-debug/jvm/target parsley-debug/native/target parsley/native/target parsley/jvm/target parsley-debug/js/target parsley/js/target project/target
- name: Upload target directories
if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master')
Something doesn't seem right with what running
prePR
did toci.yml
on dd2dc17:@@ -123,11 +123,11 @@ jobs: - name: Make target directories if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master') - run: mkdir -p parsley/native/target parsley/jvm/target parsley/js/target project/target + run: mkdir -p parsley-debug/jvm/target parsley-debug/native/target parsley/native/target parsley/jvm/target parsley-debug/js/target parsley/js/target project/target - name: Compress target directories if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master') - run: tar cf targets.tar parsley/native/target parsley/jvm/target parsley/js/target project/target + run: tar cf targets.tar parsley-debug/jvm/target parsley-debug/native/target parsley/native/target parsley/jvm/target parsley-debug/js/target parsley/js/target project/target - name: Upload target directories if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master')
Looks fine to me? What do you think is wrong with it
Something doesn't seem right with what running
prePR
did toci.yml
on dd2dc17:@@ -123,11 +123,11 @@ jobs: - name: Make target directories if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master') - run: mkdir -p parsley/native/target parsley/jvm/target parsley/js/target project/target + run: mkdir -p parsley-debug/jvm/target parsley-debug/native/target parsley/native/target parsley/jvm/target parsley-debug/js/target parsley/js/target project/target - name: Compress target directories if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master') - run: tar cf targets.tar parsley/native/target parsley/jvm/target parsley/js/target project/target + run: tar cf targets.tar parsley-debug/jvm/target parsley-debug/native/target parsley/native/target parsley/jvm/target parsley-debug/js/target parsley/js/target project/target - name: Upload target directories if: github.event_name != 'pull_request' && (startsWith(github.ref, 'refs/tags/v') || github.ref == 'refs/heads/master')
Looks fine to me? What do you think is wrong with it
The jobs list previously only had parsley
targets, and previous runs of prePR
preserved that. But now, for some reason, it decided to replace parsley
with parsley-debug
. I'm not confident if that's safe or not.
It's not replaced, it's in addition to. Just means that parsley-debug
has been marked for publish
Oh I see, I missed the fact that the parsley
targets got pushed to later in the same lines, since the diff (in my initial view) only showed the first half of the lines affected.
Current critical bug: the root tree of the debugger outputs have no children.
Edit: fixed in 8c06485
WeakMap
from ECMA 6 would be nice to have instead of this hand-rolled implementation.
If that's all you need, it is really only a few lines of code.
import scala.scalajs.js
import scala.scalajs.js.annotation.JSGlobal
@js.native
@JSGlobal
class WeakMap[K, V] extends js.Object {
def delete(key: K): Boolean = js.native
def get(key: K): js.UndefOr[V] = js.native
def has(key: K): Boolean = js.native
def set(key: K, value: V): this.type = js.native
}
On the other hand, if you need more APIs than that then it's more work. For example an IterableWeakMap
is available here:
https://github.com/typelevel/cats-effect/blob/31ad01e798133f9bc62e043aac2e7709d4ba017d/core/js/src/main/scala/cats/effect/unsafe/IterableWeakMap.scala
@armanbilge raises another interesting point. Other than the lack of scala-js implementation, we could use the java API instead of the scala one and sidestep the cross-version collections API problem (and just suffer the java API instead)...
WeakMap
from ECMA 6 would be nice to have instead of this hand-rolled implementation.If that's all you need, it is really only a few lines of code.
import scala.scalajs.js import scala.scalajs.js.annotation.JSGlobal @js.native @JSGlobal class WeakMap[K, V] extends js.Object { def delete(key: K): Boolean = js.native def get(key: K): js.UndefOr[V] = js.native def has(key: K): Boolean = js.native def set(key: K, value: V): this.type = js.native }
On the other hand, if you need more APIs than that then it's more work. For example an
IterableWeakMap
is available here: https://github.com/typelevel/cats-effect/blob/31ad01e798133f9bc62e043aac2e7709d4ba017d/core/js/src/main/scala/cats/effect/unsafe/IterableWeakMap.scala
For an implementation as simple as this is, you have to wonder why the ScalaJS team didn't decide to just add this to the standard library for it (even if it does not conform directly to Map[K, V]
)...
(For what it's worth, this API is sufficient for the task at hand. The ability to iterate over the map isn't used in the internal logic that needs the weak map.)
you have to wonder why the ScalaJS team didn't decide to just add this to the standard library for it
Because nobody got around to it yet? :) I don't see any issues open about it, and they accepted a similar PR recently in https://github.com/scala-js/scala-js/pull/4366.
Because nobody got around to it yet? :) I don't see any issues open about it, and they accepted a similar PR recently in scala-js/scala-js#4366.
I think I recall seeing this PR when looking for WeakMap
specifically, all those weeks ago
@armanbilge raises another interesting point. Other than the lack of scala-js implementation, we could use the java API instead of the scala one and sidestep the cross-version collections API problem (and just suffer the java API instead)...
This is a possibility if the interfaces are exposed to ScalaJS (Scala Native probably has them, honestly).
(At least they'd be stable, the one saving grace about Java.)
When scala-js implements the java one, it'll just work out for us (and Arman notes this may be more efficient than the scala implementation if optimisations are applied per platform). In the mean time we just need a java API facade over the top of the actual java API and make XMap... Javaish
When scala-js implements the java one, it'll just work out for us (and Arman notes this may be more efficient than the scala implementation if optimisations are applied per platform). In the mean time we just need a java API facade over the top of the actual java API and make XMap... Javaish
It seems to compile and run (using java.util.Map
and java.util.AbstractMap
) without throwing an error at runtime (like with some other Java classes), too.
For Java-ifying XMap
, I have this so far on a separate branch: coffeeification:debugger/internal/facade/XJavaMap.scala, but it currently fails to compile (compiler states no ClassTag found
for a lot of the generic variables).
The direct translations of Java's <? extends K, ? extends V>
type parameters are frankly not great to deal with ([_ <: K, _ <: V]
).
There is also the issue that the DebugTree
trait expects nodeChildren
to be a Scala Map
, and not a Java
map, so that API will end up being affected by this as well (see comment in file about Scala's collection converters):
There is one avenue to be explored, and that is manually defining a simpler Java API for some kind of "invariant map" in raw Java (that is then implemented within Scala instead).
Now the issue is, would that be cross-platform?
Ok are we ready to merge this for now? This will give us the debugger backends right, but we still need to figure out where to put the frontends (or rather, transfer them under my domain and get them ready for publishing and deploy)
It should be ready to merge. It'll give just the backend (and whatever changes to Parsley were necessary for it), the frontends still live in a separate repository on my account.
Summary
Adds a separate optional library to Parsley,
parsley-debug
. This library contains a runtime and public APIs for attaching a higher-level debugger for Parsley, to complement its lower-level debugger found inparsley.debug
.Public API
The public API is found (and documented) in
parsley.debugger.combinator
, where the basis method of this debugger (attachDebugger
) lives. This method traverses the internalLazyParsley
tree of a parser and attaches debugger nodes to every single node (even arbitrary dynamically-produced nodes fromflatMap
).This produces a pair of a generator function for a parse tree (a DebugTree; a tree representing the path of execution the parser took, along with relevant results and success statuses), and a debugged parser, which needs to be run before the generator function is called.
The tree itself has a fixed set of methods for querying information, found in
parsley.debugger.DebugTree
.For the convenience of the user, utilities are also provided in
parsley.debugger.util.Collector
, where parser names can be automatically collected from the field names found in a class (only for fields, not methods; useparsley.debugger.combinator.named
for non-field parsers). However, that uses reflection viascala-reflect
(!), which may need to be reworked (to use something else more cross-version compatible) or removed entirely.An interface,
parsley.debugger.frontend.DebugFrontend
is also provided for defining custom processing frontends for the aforementionedDebugTree
. One can traverse the tree there and do whatever they want to the tree (convert it to JSON, print it to console, display it in a GUI, etc.) as long as the entire tree is traversed. These can also be passed automatically (explicitly or via implicits) into the debugger attachment process, which can automate the processing of a tree from a parser as it runs.Known Issues or Missing Features
parsley.internal.deepembedding.frontend.debugger.DebugInjectingVisitorM
.def
-generated parsers must be manually renamed.parsley-debug
as the artifact name, with the same version asparsley
(perhaps this is not an issue).