Skip to content

Specify main classes more precisely #6980

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Aug 4, 2019
Merged

Specify main classes more precisely #6980

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0660800
Specify main classes more precisely
odersky Aug 2, 2019
f40e55c
Add missing file
odersky Aug 2, 2019
8a37267
Drop staticOnly paraaeter
odersky Aug 3, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 2 additions & 6 deletions compiler/src/dotty/tools/dotc/config/Platform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,3 @@
/* NSC -- new Scala compiler
* Copyright 2005-2012 LAMP/EPFL
* @author Paul Phillips
*/

package dotty.tools
package dotc
package config
Expand All @@ -12,6 +7,7 @@ import core.Contexts._, core.Symbols._
import core.SymbolLoader
import core.SymDenotations.SymDenotation
import core.StdNames.nme
import core.Flags.Module

/** The platform dependent pieces of Global.
*/
Expand Down Expand Up @@ -44,7 +40,7 @@ abstract class Platform {
/** The given class has a main method. */
final def hasMainMethod(sym: Symbol)(implicit ctx: Context): Boolean =
sym.info.member(nme.main).hasAltWith {
case x: SymDenotation => isMainMethod(x)
case x: SymDenotation => isMainMethod(x) && (sym.is(Module) || x.isStatic)
case _ => false
}
}
Expand Down
3 changes: 2 additions & 1 deletion compiler/src/dotty/tools/dotc/sbt/ExtractAPI.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -230,7 +230,8 @@ private class ExtractAPICollector(implicit val ctx: Context) extends ThunkHolder

allNonLocalClassesInSrc += cl

if (sym.isStatic && ctx.platform.hasMainMethod(sym)) {
if (sym.isStatic && !sym.is(Trait) && ctx.platform.hasMainMethod(sym)) {
// If sym is an object, all main methods count, otherwise only @static ones count.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this comment be moved to hasMainMethod where the test is performed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's another caller of hasMainMethod in the backend. Not knowing what
this does or expects, I am not sure whether the change
is OK or not.

Copy link
Member

Choose a reason for hiding this comment

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

So the other caller is https://github.com/lampepfl/dotty/blob/95ae77c4748bd105de45601875b447ee1fe6a98b/compiler/src/dotty/tools/dotc/transform/CollectEntryPoints.scala#L66-L67
which will emit a warning if there's a main method in an object and also a main method in the companion class of the object, and we do want the warning even if the main method in the companion class is not static, since its mere presence means that the backend will not generate a static forwarder from the main in the object to the companion class.

Copy link
Member

Choose a reason for hiding this comment

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

The situation is very confusing because there's two source files called CollectEntryPoints and none of them actually work, I've made a separate PR to cleanup things a bit: 89801f9

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@smarter Do you want to take this over, or should we merge as is? I don't have the time to do more work on this.

_mainClasses += name
}

Expand Down