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

SIP: Language support for conditional compilation #1541

Closed
wants to merge 3 commits into from

Conversation

szeiger
Copy link
Contributor

@szeiger szeiger commented Oct 7, 2019

Someone please check the Jekyll output. I managed to build it and the file shows up under /sips/sips/2019-10-07-preprocessor-for-conditional-compilation.html and looks OK but it does not appear in the SIP list or get a URL without the date prefix. Jekyll's verbose logging is of no help. No errors are shown.

@lrytz
Copy link
Member

lrytz commented Oct 7, 2019

Adapted the jekyll front matter to look like other SIPs, now it shows up in the overview page and gets a clean URL.

Copy link
Contributor

@olafurpg olafurpg left a comment

Choose a reason for hiding this comment

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

Thank you for writing up this proposal. As I mentioned in scala/scala-dev#640 (comment), I think this proposal is a serious step backwards and I hope it won't be accepted.

I believe that cross-building via source files/directories is the best solution to solve the problem statement of this proposal. I would like to see a better analysis/motivation for why cross-building via source files/directories is a prohibitive solution.


## Motivation

When upgrading projects from Scala 2.12 to 2.13 the changes to the collections library often require minor changes to method signatures and implementation details. This poses a problem for cross-building which is usually required when upgrading a library rather than an application). The [scala-collection-compat](https://github.com/scala/scala-collection-compat) library cannot eliminate all incompatibilities. For the remaining ones the proposed solution consists of separate source directories which limits the differences between cross-builds to entire compilation units. In order to keep code duplication to a minimum this will usually lead to seemingly arbitrary and unnecessary design decisions that are driven solely by the need to cross-build. For example, in scala-collection-compat itself the [PackageShared](https://github.com/scala/scala-collection-compat/blob/master/compat/src/main/scala-2.12/scala/collection/compat/package.scala#L17) trait only exists to limit code duplication between the 2.11 and 2.12 versions, the only difference being two extra methods in the 2.12 version.
Copy link
Contributor

Choose a reason for hiding this comment

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

arbitrary and unnecessary design decisions that are driven solely by the need to cross-build

Could you somehow quantify or justify how big of a pain point this is in practice? For example, in what way has the introduction of PackageShared caused problems?

The motivation for this SIP proposal build on the claim that cross-building by source files/directories is prohibitive but the provided PackagedShared example doesn't demonstrate that point in my opinion. My personal experience is that it's annoying to introduce superfluous traits and objects to support cross-building but it rarely causes major problems.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course it is never prohibitive. Every conditional compilation situation can be solved with separate source directories. We propose a way of making it easier and less repetitive.


When upgrading projects from Scala 2.12 to 2.13 the changes to the collections library often require minor changes to method signatures and implementation details. This poses a problem for cross-building which is usually required when upgrading a library rather than an application). The [scala-collection-compat](https://github.com/scala/scala-collection-compat) library cannot eliminate all incompatibilities. For the remaining ones the proposed solution consists of separate source directories which limits the differences between cross-builds to entire compilation units. In order to keep code duplication to a minimum this will usually lead to seemingly arbitrary and unnecessary design decisions that are driven solely by the need to cross-build. For example, in scala-collection-compat itself the [PackageShared](https://github.com/scala/scala-collection-compat/blob/master/compat/src/main/scala-2.12/scala/collection/compat/package.scala#L17) trait only exists to limit code duplication between the 2.11 and 2.12 versions, the only difference being two extra methods in the 2.12 version.

While the collection changes are the most prominent use case at the moment, breaking changes have occured in the past and are expected to continue, particularly with the upgrade to Scala 3. Since adoption of new Scala versions benefits from a speedy upgrade of libraries developed by the Scala community, it is in Scala's best interest to make this cross-building scenario as easy as possible, even if only a limited number of developers will use it *directly*.
Copy link
Contributor

Choose a reason for hiding this comment

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

How do you ensure that pre-processing continues to only be used for the originally intended purpose by a limited number of developers?

My personal observation is that "power features" (example: macros) have a tendency of becoming widely used in creative ways.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be made clearer in the updated version (coming shortly). The feature set is so limited that there's simply nothing else you can do with it.


Third-party tooling based on the standard Scala parser is expected to continue to work without major changes. Custom parser implementations need to be updated to support the preprocessor syntax. Ignoring the preprocessor after parsing is a sensible default in many cases. For example, an IDE could treat any omitted code block like a comment. This is similar to the current situation with separate source directories where only one set of directories, corresponding to the main build version, contains the sources on which the IDE operates.

Some source-based tools will face bigger difficulties. In particular, source rewriting becomes much more challenging in the presence of conditional code blocks, especially if they do not have to conform to the AST's nesting structure.
Copy link
Contributor

Choose a reason for hiding this comment

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

Some source-based tools will face bigger difficulties

It would be good give concrete examples of what tools may be affected by this proposal:

  • parsers will need to be updated: Fastparse, IntelliJ, Scalameta, Scalariform
  • syntax highlighters will need to be updated
  • semantic refactoring tools like IntelliJ and Scalafix will need to rethink core assumptions in order to support basic functionality like "rename symbol" to work in pre-processed files
  • IDEs like IntelliJ and Metals will need to rethink core assumptions to support features like "goto definition" and "fold range" in pre-processed files
  • code formatting tools like Scalafmt may need to understand syntax trees that contain multiple Scala dialects in the same source file (it's not possible to treat ignored pre-processed blocks as code comments)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMHO most of these fall under "continue to work without major changes". Third-party parsers, syntax highlighters and IDEs need to support the new syntax but are not expected to work on multiple conditional versions at the same time. I hacked the parser and interpreter for scalac together in a few days (without much previous experience working on the scalac parser) so I don't expect this to be a big problem.

The configuration parameters determine which code blocks are active. Any inactive block can be treated the same way as a comment. As mentioned in the first paragraph, this gives you the same level of support that you would get with the current directory-based approach. Anything beyond that would be nice to have, but is not a requirement for supporting the new conditional compilation scheme.

The situation for semantic refactoring tools is similiar: they could work on one conditional version, all others would be considered comments and run the risk of getting out of sync. You already have the same limitation with directory-based conditional compilation.

Code formatting tools are affected the most because they work well with directory-based conditional compilation. You would have to do multiple passes over a file for the different configurations. Things may get more difficult if reformatting needs to move things around across multiple lines.

@SethTisue
Copy link
Member

SethTisue commented Nov 14, 2019

as we're discussing by the pool 🏊‍♂️, the conflict with the indentation-based syntax doesn't bother me. the "less desirable" version, where the branches aren't indented, seems fine to me; I had not expected to be able to additionally indent inside #if

and as we're also discussing, it might even be best to require that the preprocessor directives be all the way at column 0, as in C. it loudly signals that it's line-based preprocessor stuff happening (and might make it easier for tooling to handle?)

After review comments and discussions, I made some changes to the
document, in particular:
- Renamed to "Language support for conditional compilation"
- De-emphasized the use of the term "preprocessor" throughout the
  document. While this is how the implementation works, it gave people
  the wrong impression about what this feature can and cannot do.
- Expanded the "Tooling" section
- Moved "Related Work" to the end
- Require directives to start in the first column. This avoids the
  problems with indentation-based syntax. I am personally not a fan of
  this style but others seem to be less bothered by it.
@lrytz lrytz changed the title SIP: Preprocessor for conditional compilation SIP: Language support for conditional compilation Nov 26, 2019
@SethTisue SethTisue marked this pull request as draft April 30, 2020 03:12
@SethTisue
Copy link
Member

closing for inactivity. could be revived

@SethTisue SethTisue closed this Oct 19, 2020
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.

4 participants