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

SI-6811 Move scala.util.{automata,regexp} ... #1939

Merged
merged 1 commit into from Jan 25, 2013

Conversation

Projects
None yet
5 participants
@soc
Copy link
Member

soc commented Jan 20, 2013

... to scala.xml.dtd.impl and make it private[dtd]

@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 20, 2013

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2187/

@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 20, 2013

jenkins job pr-scala-testsuite-linux-opt: Failed - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2187/

@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 20, 2013

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 20, 2013

PLS REBUILD ALL

@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 20, 2013

SI-6811 Move scala.util.{automata,regexp} ...
... to scala.xml.dtd.impl and make it private[dtd]
@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 21, 2013

Started jenkins job pr-scala-testsuite-linux-opt at https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2189/

@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 21, 2013

jenkins job pr-scala-testsuite-linux-opt: Success - https://scala-webapps.epfl.ch/jenkins/job/pr-scala-testsuite-linux-opt/2189/

@paulp

This comment has been minimized.

Copy link
Contributor

paulp commented Jan 21, 2013

Excellent. Looks good to me. If there is more to do to make it possible to break off the xml capabilities (I can't remember) it would be a super useful thing to do - I could break out scala-xml.jar for 2.11 with the library and hopefully the xml parser as well.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 21, 2013

@paulp I think the hardest part now is to get rid of the infamous $scope value in Predef: https://github.com/scala/scala/blob/master/src/library/scala/Predef.scala#L137

Sadly, I have no idea how it ended up there, what it does, how it works, etc. Looking at the history, it seems you are the expert here (even if it only involved moving it from file to file). :-)

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 21, 2013

Just a left-field idea: I think we could break the dependency by making $scope a macro. If the required types from scala-xml.jar are on the compilation classpath, it would expand into something compatible with what we have today.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 21, 2013

I was just looking at https://github.com/scala/scala/blob/master/src/compiler/scala/tools/nsc/ast/parser/SymbolicXMLBuilder.scala and I think moving $scope from Predef to scala/xml/package.scala could work.

What would be the benefit of a macro?

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 21, 2013

In the current design, the default $scope identifier has to be imported at the start of each file (Predef being the only way we have to do that.)

When a new namespace binding is detected, this value is shadowed. See below for an example.

ticket/6968 ~/code/scala2 scala -Xprint:parser -e '<foo><bar xmlns="ns1">{<baz/>}</bar></foo>'
[[syntax trees at end of parser]]// Scala source: scalacmd4625646308710851260.scala
package <empty> {
  object Main extends scala.ScalaObject {
    def <init>() = {
      super.<init>();
      ()
    };
    def main(argv: Array[String]): scala.Unit = {
      val args = argv;
      {
        final class $anon extends scala.AnyRef {
          def <init>() = {
            super.<init>();
            ()
          };
          {
            {
              new _root_.scala.xml.Elem(null, "foo", _root_.scala.xml.Null, $scope, ({
                val $buf = new _root_.scala.xml.NodeBuffer();
                $buf.$amp$plus({
                  var $tmpscope: _root_.scala.xml.NamespaceBinding = $scope;
                  $tmpscope = new _root_.scala.xml.NamespaceBinding(null, "ns1", $tmpscope);
                  {
                    val $scope: _root_.scala.xml.NamespaceBinding = $tmpscope;
                    new _root_.scala.xml.Elem(null, "bar", _root_.scala.xml.Null, $scope, ({
                      val $buf = new _root_.scala.xml.NodeBuffer();
                      $buf.$amp$plus({
                        {
                          new _root_.scala.xml.Elem(null, "baz", _root_.scala.xml.Null, $scope)
                        }
                      });
                      $buf
                    }: _*))
                  }
                });
                $buf
              }: _*))
            }
          }
        };
        new $anon()
      }
    }
  }
}

ticket/6968 ~/code/scala2 

I haven't thought about it too much, but you might like to consider an alternative design in which the parser inserts an import when it encounters the first XML nodes in the tree, or maybe just records whether there were any XML nodes in the tree at all, and adds import xml.$scope just below the Predef import as a post-processing step.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 21, 2013

Mhhh, can't we just emit somthing like scala.xml.$scope instead? Sorry if this is stupid, I'm not familiar with the mechanism.

What about requiring an import before using XML, e. g. scala.xml? Wouldn't that also be more friendly to other XML libraries, which could just hook into this mechanism?

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 22, 2013

You can't emit a fully qualified scala.xml.$scope, because as soon as you are nested in a namespace declaration, it must bind to the nearest lexically enclosing val $scope, if you read that desugared code carefully you should see how it works.

Here's a sketch of what I mean: https://github.com/retronym/scala/compare/topic/xml-predef

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 22, 2013

@soc btw, beyond that compiler patch, the task of splitting out scala-xml.jar amounts to hacking the Ant and SBT builds, the Eclipse/IntelliJ project defintions, most likely the SBT build in the scala-dist project, and writing some documentation. So, not meaning to dampen your enthusiasm, it probably isn't very suitable project for an external contribution. But it is a priority for us for 2.11, so we won't forget about it.

@retronym retronym closed this Jan 22, 2013

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 22, 2013

Why was this closed?

The changes in this PR need to be done in one way or another, and they don't depend on any specific approach to $scope.

@retronym retronym reopened this Jan 22, 2013

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 22, 2013

Sorry, wrong button.

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 22, 2013

No problem! Your sketch looks pretty interesting, I tried to hack SymbolicXMLBuilder earlier with very limited success.

I'm wondering whether there is a more general approach, which would allow third-party libraries to hook into the syntax.

Would it be possible to have some sort of interface which would not build the XML itself, but just return events to an implementation of that interface found on the classpath? Sounds a bit like a pretty standard macro with a special-cased extension.

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 22, 2013

It's an appealing idea, but I don't think we have the resources to open up additional extension points like that. There are a lot of downstream considerations when allowing syntax to be customised -- the IDEs, editors, syntax highlighters etc wouldn't work for these dialects.

@harrah

This comment has been minimized.

Copy link
Contributor

harrah commented Jan 22, 2013

My understanding is that soc is not recommending customizing the syntax, but wants to be able to plug in a different xml library. So, the syntax is the same, but perhaps the resulting type is different.

In any case, I have wasted many hours trying to keep Proguard from pulling in xml or other packages. It has mainly been because of things in Predef or the scala package object. I'll be happy to see this go!

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Jan 22, 2013

Oh right. I expect Paul has a plan, given his stated modularlization of "hopefully the xml parser as well."

@soc

This comment has been minimized.

Copy link
Member Author

soc commented Jan 22, 2013

@harrah Exactly! Basically some interface with which the library can “subscribe” to the “events” of the XML parser (like “new tag foo encountered”) and provides the appropriate AST to the compiler in return.

Or maybe it could be specified that the XML is turned into xml"""...""" and that people can choose the library by importing/shadowing the xml string interpolator with the one they want. (But as far as I have heard, there are certain issues with rewriting XML to string interpolation...)

@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 24, 2013

1 similar comment
@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 24, 2013

@scala-jenkins

This comment has been minimized.

Copy link

scala-jenkins commented Jan 24, 2013

paulp added a commit that referenced this pull request Jan 25, 2013

Merge pull request #1939 from soc/SI-6811-moves
SI-6811 Move scala.util.{automata,regexp} ...

@paulp paulp merged commit f5d5139 into scala:master Jan 25, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment