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

Refactor CrystalPath::Error #9359

Merged
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
44 changes: 16 additions & 28 deletions spec/compiler/crystal_path/crystal_path_spec.cr
Expand Up @@ -4,19 +4,23 @@ require "../../support/env"
private def assert_finds(search, results, relative_to = nil, path = __DIR__, file = __FILE__, line = __LINE__)
it "finds #{search.inspect}", file, line do
crystal_path = Crystal::CrystalPath.new(path)
relative_to = "#{__DIR__}/#{relative_to}" if relative_to
results = results.map { |result| "#{__DIR__}/#{result}" }
matches = crystal_path.find search, relative_to: relative_to
matches.should eq(results)
results = results.map { |result| File.join(__DIR__, result) }
Dir.cd(__DIR__) do
matches = crystal_path.find search, relative_to: relative_to
matches.should eq(results), file: file, line: line
end
end
end

private def assert_doesnt_find(search, relative_to = nil, path = __DIR__, file = __FILE__, line = __LINE__)
private def assert_doesnt_find(search, relative_to = nil, path = __DIR__, expected_relative_to = nil, file = __FILE__, line = __LINE__)
it "doesn't finds #{search.inspect}", file, line do
crystal_path = Crystal::CrystalPath.new(path)
relative_to = "#{__DIR__}/#{relative_to}" if relative_to
expect_raises Exception, /can't find file/ do
crystal_path.find search, relative_to: relative_to
Dir.cd(__DIR__) do
error = expect_raises Crystal::CrystalPath::NotFoundError do
crystal_path.find search, relative_to: relative_to
end
error.relative_to.should eq(expected_relative_to), file: file, line: line
error.filename.should eq(search), file: file, line: line
end
end
end
Expand Down Expand Up @@ -88,33 +92,17 @@ describe Crystal::CrystalPath do

assert_doesnt_find "file_two.cr"
assert_doesnt_find "test_folder/file_three.cr"
assert_doesnt_find "test_folder/*", relative_to: "#{__DIR__}/test_files/file_one.cr"
assert_doesnt_find "test_folder/*", relative_to: Path[__DIR__, "test_files", "file_one.cr"].to_s, expected_relative_to: Path[__DIR__, "test_files"].to_s
assert_doesnt_find "test_files/missing_file.cr"
assert_doesnt_find __FILE__[1..-1], path: ":"

# Don't find in CRYSTAL_PATH if the path is relative (#4742)
assert_doesnt_find "./crystal_path_spec", relative_to: "test_files/file_one.cr"
assert_doesnt_find "./crystal_path_spec.cr", relative_to: "test_files/file_one.cr"
assert_doesnt_find "./crystal_path_spec", relative_to: Path["test_files", "file_one.cr"].to_s, expected_relative_to: Path["test_files"].to_s
assert_doesnt_find "./crystal_path_spec.cr", relative_to: Path["test_files", "file_one.cr"].to_s, expected_relative_to: Path["test_files"].to_s
assert_doesnt_find "../crystal_path/test_files/file_one"

# Don't find relative filenames in src or shards
assert_doesnt_find "../../src/file_three", relative_to: "test_files/test_folder/test_folder.cr"

it "prints an explanatory message for non-relative requires" do
crystal_path = Crystal::CrystalPath.new(__DIR__)
ex = expect_raises Exception, /If you're trying to require a shard/ do
crystal_path.find "non_existent", relative_to: __DIR__
end
end

it "doesn't print an explanatory message for relative requires" do
crystal_path = Crystal::CrystalPath.new(__DIR__)
ex = expect_raises Exception, /can't find file/ do
crystal_path.find "./non_existent", relative_to: __DIR__
end

ex.message.not_nil!.should_not contain "If you're trying to require a shard"
end
assert_doesnt_find "../../src/file_three", relative_to: Path["test_files", "test_folder", "test_folder.cr"].to_s, expected_relative_to: Path["test_files", "test_folder"].to_s

it "includes 'lib' by default" do
with_env("CRYSTAL_PATH": nil) do
Expand Down
35 changes: 31 additions & 4 deletions spec/compiler/semantic/require_spec.cr
@@ -1,10 +1,37 @@
require "../../spec_helper"

describe "Semantic: require" do
it "raises crystal exception if can't find require (#7385)" do
node = parse(%(require "file_that_doesnt_exist"))
expect_raises ::Crystal::Exception do
semantic(node)
describe "file not found" do
it "require" do
error = assert_error %(require "file_that_doesnt_exist"),
"can't find file 'file_that_doesnt_exist'",
inject_primitives: false

error.message.not_nil!.should contain "If you're trying to require a shard:"
end

it "relative require" do
error = assert_error %(require "./file_that_doesnt_exist"),
"can't find file './file_that_doesnt_exist'",
inject_primitives: false

error.message.not_nil!.should_not contain "If you're trying to require a shard:"
end

it "wildecard" do
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
error = assert_error %(require "file_that_doesnt_exist/*"),
"can't find file 'file_that_doesnt_exist/*'",
inject_primitives: false

error.message.not_nil!.should contain "If you're trying to require a shard:"
end

it "relative wildecard" do
straight-shoota marked this conversation as resolved.
Show resolved Hide resolved
error = assert_error %(require "./file_that_doesnt_exist/*"),
"can't find file './file_that_doesnt_exist/*'",
inject_primitives: false

error.message.not_nil!.should_not contain "If you're trying to require a shard:"
end
end
end
29 changes: 9 additions & 20 deletions src/compiler/crystal/crystal_path.cr
Expand Up @@ -3,7 +3,12 @@ require "./exception"

module Crystal
struct CrystalPath
class Error < LocationlessException
class NotFoundError < LocationlessException
getter filename
getter relative_to

def initialize(@filename : String, @relative_to : String?)
end
end

private DEFAULT_LIB_PATH = "lib"
Expand Down Expand Up @@ -46,7 +51,9 @@ module Crystal
result = find_in_crystal_path(filename)
end

cant_find_file filename, relative_to unless result
unless result
raise NotFoundError.new(filename, relative_to)
end

result = [result] if result.is_a?(String)
result
Expand Down Expand Up @@ -159,23 +166,5 @@ module Crystal

nil
end

private def cant_find_file(filename, relative_to)
error = "can't find file '#{filename}'"

if filename.starts_with? '.'
error += " relative to '#{relative_to}'" if relative_to
else
error = <<-NOTE
#{error}

If you're trying to require a shard:
- Did you remember to run `shards install`?
- Did you make sure you're running the compiler in the same directory as your shard.yml?
NOTE
end

raise Error.new(error)
end
end
end
2 changes: 1 addition & 1 deletion src/compiler/crystal/macros/macros.cr
Expand Up @@ -158,7 +158,7 @@ class Crystal::Program
begin
files = @program.find_in_path(recorded_require.filename, recorded_require.relative_to)
required_files.concat(files) if files
rescue Crystal::CrystalPath::Error
rescue Crystal::CrystalPath::NotFoundError
# Maybe the file is gone
next
end
Expand Down
18 changes: 18 additions & 0 deletions src/compiler/crystal/semantic/semantic_visitor.cr
Expand Up @@ -68,6 +68,24 @@ abstract class Crystal::SemanticVisitor < Crystal::Visitor
node.expanded = expanded
node.bind_to(expanded)
false
rescue ex : CrystalPath::NotFoundError
Copy link
Member

Choose a reason for hiding this comment

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

Can we move this rescue just around find_in_path because that's where the error can happen? This makes it much clearer when the exception is expected to happen.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered that, but the problem is that the exception raised there would subsequently be rescued in line 89. I don't think it makes sense to make that rescue handler work around that.
A slightly less confusing option could be to create the error in a rescue around find_in_path, assign it to a local variable and only raise in an else rescue handler. Not sure if that's worth it, though.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, I see. Yeah, let's leave it like that for now.

Copy link
Member

Choose a reason for hiding this comment

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

That said, I don't understand why CrystalPath::NotFoundError is handled in a special way here, instead of just being a regular Crystal::Exception and having a sensible to_s.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea was to have CrystalPath::NotFoundError as a plain internal error type, not related to any user code interpretation. CrystalPath should not care wheter it was invoked for a require or something else. It also doesn't know the code location, so it can't really create a proper user-facing error.
NotFoundError just communicates that finding a path failed. What that means depends on the context. In the context of interpreting a Require node it means the require has failed and an appropriate error is raised for that specific situation.

message = "can't find file '#{ex.filename}'"
notes = [] of String

# FIXME: as(String) should not be necessary
if ex.filename.as(String).starts_with? '.'
if relative_to
message += " relative to '#{relative_to}'"
end
else
notes << <<-NOTE
If you're trying to require a shard:
- Did you remember to run `shards install`?
- Did you make sure you're running the compiler in the same directory as your shard.yml?
NOTE
end

node.raise "#{message}\n\n#{notes.join("\n")}"
rescue ex : Crystal::Exception
node.raise "while requiring \"#{node.string}\"", ex
rescue ex
Expand Down