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.

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

fix duplicate include statements #256

Closed
wants to merge 3 commits into from
+42 −25
Closed

fix duplicate include statements #256

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
25ef670
fix duplicate include statements when importing same protofiles in di…
arnemohr Nov 14, 2024
deb0c01
add JET 0.9 to compat versions
arnemohr Nov 14, 2024
961a8f1
re-add removed comments
arnemohr Nov 15, 2024
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
2 changes: 1 addition & 1 deletion Project.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Aqua = "0.8"
BufferedStreams = "1.2"
Dates = "1"
EnumX = "1"
JET = "0.4, 0.5, 0.6, 0.7, 0.8"
JET = "0.4, 0.5, 0.6, 0.7, 0.8, 0.9"
TOML = "1"
Test = "1"
julia = "1.6"
Expand Down
36 changes: 23 additions & 13 deletions src/codegen/modules.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ struct ProtoModule
nonpkg_imports::Set{String}
end
namespace(m::ProtoModule) = m.namespace
empty_module(name::AbstractString, namespace_vec) = ProtoModule(name, namespace_vec, [], Dict(), Set(), Set(), Set())
empty_module(name::AbstractString, namespace_vec) = ProtoModule(name, namespace_vec, [], Dict(), Set(), Set(), Set())

struct Namespaces
non_namespaced_protos::Vector{ResolvedProtoFile}
Expand All @@ -64,7 +64,7 @@ function Namespaces(files_in_order::Vector{ResolvedProtoFile}, root_path::String
push!(t.non_namespaced_protos, file)
else
top_namespace = first(namespace(file))
p = get!(()->empty_module(top_namespace, [top_namespace]), t.packages, top_namespace)
p = get!(() -> empty_module(top_namespace, [top_namespace]), t.packages, top_namespace)
_add_file_to_package!(p, file, proto_files, root_path)
end
end
Expand All @@ -78,7 +78,7 @@ function _add_file_to_package!(root::ProtoModule, file::ResolvedProtoFile, proto
depth += 1
name == node.name && continue
_ns = namespace(file)[1:depth]
node = get!(()->empty_module(name, _ns), node.submodules, _ns)
node = get!(() -> empty_module(name, _ns), node.submodules, _ns)
end
for ipath in import_paths(file)
imported_file = proto_files[ipath]
Expand All @@ -87,7 +87,7 @@ function _add_file_to_package!(root::ProtoModule, file::ResolvedProtoFile, proto
# Sometimes they are forced to be namespaced with `always_use_modules`
# but it is the responsibility of the root module to make sure there
# is a importable module in to topmost scope
depth != 1 && push!(node.external_imports, string("." ^ depth, replace(proto_script_name(imported_file), ".jl" => "")))
depth != 1 && push!(node.external_imports, string("."^depth, replace(proto_script_name(imported_file), ".jl" => "")))
push!(root.nonpkg_imports, replace(proto_script_name(imported_file), ".jl" => ""))
else
file_pkg = proto_package_name(imported_file)
Expand All @@ -96,7 +96,7 @@ function _add_file_to_package!(root::ProtoModule, file::ResolvedProtoFile, proto
elseif file_pkg == root.name # same package, different submodule
push!(node.internal_imports, namespace(imported_file))
else
depth != 1 && push!(node.external_imports, string("." ^ depth, proto_package_name(imported_file)))
depth != 1 && push!(node.external_imports, string("."^depth, proto_package_name(imported_file)))
push!(root.external_imports, rel_import_path(imported_file, root_path))
end
end
Expand All @@ -108,6 +108,9 @@ end
function generate_module_file(io::IO, m::ProtoModule, output_directory::AbstractString, parsed_files::Dict, options::Options, depth::Int)
println(io, "module $(m.name)")
println(io)

included_files = Set{String}()

has_deps = !isempty(m.internal_imports) || !isempty(m.external_imports) || !isempty(m.nonpkg_imports)
if depth == 1
# This is where we include external packages so they are available downstream
Expand All @@ -118,9 +121,12 @@ function generate_module_file(io::IO, m::ProtoModule, output_directory::Abstract
# We wrap them in a module to make sure that multiple downstream dependencies
# can import them safely.
for nonpkg_import in m.nonpkg_imports
!options.always_use_modules && print(io, "module $(nonpkg_import)\n ")
println(io, "include(", repr(joinpath("..", string(nonpkg_import, ".jl"))), ')')
!options.always_use_modules && println(io, "end")
if !(nonpkg_import in included_files)
!options.always_use_modules && print(io, "module $(nonpkg_import)\n ")
println(io, "include(", repr(joinpath("..", string(nonpkg_import, ".jl"))), ')')
!options.always_use_modules && println(io, "end")
push!(included_files, nonpkg_import)
end
end
else # depth > 1
# We're not a top package module, we can import external dependencies
Expand All @@ -145,11 +151,11 @@ function generate_module_file(io::IO, m::ProtoModule, output_directory::Abstract
# This is where we import internal dependencies. We only need to import the toplevel package
# and then use fully qualified names for the imported definitions.
if !isempty(m.internal_imports)
print(io, "import ", string("." ^ length(namespace(m)), first(namespace(m))))
print(io, "import ", string("."^length(namespace(m)), first(namespace(m))))
# We can't import the toplevel module to a leaf module if their names collide
# we also need to tweak the `package_namespace` field of each imported ReferencedType
if first(namespace(m)) == last(namespace(m))
println(io, " as var\"#", first(namespace(m)), '"')
println(io, " as var\"#", first(namespace(m)), '"')
else
println(io)
end
Expand All @@ -158,6 +164,7 @@ function generate_module_file(io::IO, m::ProtoModule, output_directory::Abstract
# Load in imported proto files that are defined in this package (the files ending with `_pb.jl`)
# In case there is a dependency of some of these files on a submodule, we include that submodule
# first.

seen = Set{String}()
for file in m.proto_files
for i in import_paths(file)
Expand All @@ -169,7 +176,10 @@ function generate_module_file(io::IO, m::ProtoModule, output_directory::Abstract
end
end
end
println(io, "include(", repr(proto_script_name(file)), ")")
if !(proto_script_name(file) in included_files)
println(io, "include(", repr(proto_script_name(file)), ")")
push!(included_files, proto_script_name(file))
end
end
# Load in submodules nested in this namespace (the modules ending with `PB`),
# that is, if we didn't include them above.
Expand All @@ -194,14 +204,14 @@ function generate_package(node::ProtoModule, output_directory::AbstractString, p
CodeGenerators.translate(dst_path, file, parsed_files, options)
end
for submodule in values(node.submodules)
generate_package(submodule, path, parsed_files, options, depth+1)
generate_package(submodule, path, parsed_files, options, depth + 1)
end
return nothing
end

function validate_search_directories!(search_directories::Vector{String}, include_vendored_wellknown_types::Bool)
include_vendored_wellknown_types && push!(search_directories, VENDORED_WELLKNOWN_TYPES_PARENT_PATH)
unique!(map!(x->joinpath(abspath(x), ""), search_directories, search_directories))
unique!(map!(x -> joinpath(abspath(x), ""), search_directories, search_directories))
bad_dirs = filter(!isdir, search_directories)
!isempty(bad_dirs) && error("`search_directories` $bad_dirs don't exist")
return nothing
Expand Down
29 changes: 18 additions & 11 deletions src/codegen/toplevel_definitions.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,13 +18,13 @@
# is already a vector if the field was repeated.
type_param = get(type_params, field.name, nothing)
if struct_name == type_name
type_name = string("Union{Nothing,", type_name,"}")
type_name = string("Union{Nothing,", type_name, "}")
elseif !isnothing(type_param)
type_name = _is_repeated_field(field) ? string("Vector{", type_param.param, '}') : type_param.param
elseif field.label == Parsers.OPTIONAL || field.label == Parsers.DEFAULT
should_force_required = _should_force_required(string(struct_name, ".", field.name), ctx)
if !should_force_required && _is_message(field.type, ctx)
type_name = string("Union{Nothing,", type_name,"}")
type_name = string("Union{Nothing,", type_name, "}")
end
end
println(io, " ", field_name, "::", type_name)
Expand All @@ -40,13 +40,13 @@
# is already a vector if the field was repeated.
type_param = get(type_params, field.name, nothing)
if struct_name == type_name
type_name = string("Union{Nothing,", type_name,"}")
type_name = string("Union{Nothing,", type_name, "}")

Check warning on line 43 in src/codegen/toplevel_definitions.jl

View check run for this annotation

Codecov / codecov/patch

src/codegen/toplevel_definitions.jl#L43 

Added line #L43 was not covered by tests
elseif !isnothing(type_param)
type_name = type_param.param
elseif field.label == Parsers.OPTIONAL || field.label == Parsers.DEFAULT
should_force_required = _should_force_required(string(struct_name, ".", field.name), ctx)
if !should_force_required
type_name = string("Union{Nothing,", type_name,"}")
type_name = string("Union{Nothing,", type_name, "}")
end
end
println(io, " ", field_name, "::", type_name)
Expand Down Expand Up @@ -101,7 +101,7 @@
generate_struct(io, t, ctx)
maybe_generate_kwarg_constructor_method(io, t, ctx)
maybe_generate_deprecation(io, t)
maybe_generate_reserved_fields_method(io, t )
maybe_generate_reserved_fields_method(io, t)
maybe_generate_extendable_field_numbers_method(io, t)
maybe_generate_oneof_field_types_method(io, t, ctx)
maybe_generate_default_values_method(io, t, ctx)
Expand Down Expand Up @@ -149,10 +149,12 @@
p = rp.proto_file
Parsers.check_name_collisions(p, file_map)
println(io, "# Autogenerated using ProtoBuf.jl v$(PACKAGE_VERSION) on $(Dates.now())")
println(io, "# original file: ", p.filepath," (proto", p.preamble.isproto3 ? '3' : '2', " syntax)")
println(io, "# original file: ", p.filepath, " (proto", p.preamble.isproto3 ? '3' : '2', " syntax)")
println(io)

# TODO: cleanup here, we probably don't need a reference to rp.transitive_imports in ctx?
included_files = Set{String}()

ctx = Context(p, rp.import_path, file_map, copy(p.cyclic_definitions), Ref{String}(), rp.transitive_imports, options)
if !is_namespaced(p)
options.always_use_modules && println(io, "module $(replace(proto_script_name(p), ".jl" => ""))")
Expand All @@ -162,14 +164,19 @@
for path in import_paths(p)
dependency = file_map[path]
if !is_namespaced(dependency)
# if the dependency is also not namespaced, we can just include it
println(io, "include(", repr(proto_script_name(dependency)), ")")
options.always_use_modules && println(io, "import $(replace(proto_script_name(dependency), ".jl" => ""))")
if !(proto_script_name(dependency) in included_files)
println(io, "include(", repr(proto_script_name(dependency)), ")")
options.always_use_modules && println(io, "import $(replace(proto_script_name(dependency), ".jl" => ""))")
push!(included_files, proto_script_name(dependency))
end
else
# otherwise we need to import it trough a module
import_pkg_name = namespaced_top_import(dependency)
println(io, "include(", repr(namespaced_top_include(dependency)), ")")
println(io, "import $(import_pkg_name)")
if !(import_pkg_name in included_files)
println(io, "include(", repr(namespaced_top_include(dependency)), ")")
println(io, "import $(import_pkg_name)")
push!(included_files, import_pkg_name)
end
end
end
end # Otherwise all includes will happen in the enclosing module
Expand Down
Loading