Skip to content

Harden IDE: Catch NoSuchFileException #4002

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

Merged
merged 2 commits into from
Feb 27, 2018
Merged

Conversation

odersky
Copy link
Contributor

@odersky odersky commented Feb 15, 2018

A NoSuchFileException was observed for a non-existing directory on the classpath:

/Users/odersky/workspace/dotty/compiler/../out/bootstrap/dotty-compiler-bootstrapped/scala-0.7/classes

It was after a dotty-bootstrapped/clean. I believe this should be ignored, or maybe handled with
an error message. But certainly not a crash.

A NoSuchFileException was observed for a non-existing directory on the classpath:

    /Users/odersky/workspace/dotty/compiler/../out/bootstrap/dotty-compiler-bootstrapped/scala-0.7/classes

It was after a dotty-bootstrapped/clean. I believe this should be ignored, or maybe handled with
an error message. But certainly not a crash.
})
})
catch {
case _: NoSuchFileException =>
Copy link
Contributor

Choose a reason for hiding this comment

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

We should emit an error or warning. If we don't add it in this PR we might have silent failures that are impossible to debug.

if name.endsWith(tastySuffix)
} {
names += root.relativize(path).toString.replace("/", ".").stripSuffix(tastySuffix)
try
Copy link
Contributor

Choose a reason for hiding this comment

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

if (Files.exists(root)) ... instead of catching an exception?

@sjrd
Copy link
Member

sjrd commented Feb 15, 2018

The standard behavior is to (silently) ignore non-existent entries in a classpath. Both scalac and the JVM do that, and historically I can tell you that people rely on this behavior because we had a bug report in Scala.js reporting that we did not ignore them: scala-js/scala-js#2198

@odersky odersky dismissed nicolasstucki’s stale review February 17, 2018 17:34

Is contradicted by @sjrd's comment

Paths can contain non-tree elements. I observed a path that contained
an untpd.Mod instead of a tree.

Also, we should survive possible cyclic references when ccomputing the context
of a path. (this was also observed in the wild but I can't reproduce it).
@@ -344,7 +344,7 @@ object Interactive {
if (tree.pos.contains(pos)) {
// FIXME: We shouldn't need a cast. Change NavigateAST.pathTo to return a List of Tree?
val path = NavigateAST.pathTo(pos, tree, skipZeroExtent = true).asInstanceOf[List[untpd.Tree]]
path.dropWhile(!_.hasType).asInstanceOf[List[tpd.Tree]]
path.dropWhile(!_.hasType) collect { case t: tpd.Tree @unchecked => t }
Copy link
Contributor

Choose a reason for hiding this comment

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

If path can contain non-tree elements then hasType will also throws. Maybe change NavigateAST.pathTo to discard non-tree element and return a List[Tree] instead (as suggested in the comment).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point.

Copy link
Contributor Author

@odersky odersky Feb 21, 2018

Choose a reason for hiding this comment

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

I made the change on the other PR #3967

@odersky odersky requested a review from Duhemm February 21, 2018 09:27
@odersky odersky merged commit 579de32 into scala:master Feb 27, 2018
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.

5 participants