Skip to content

Include generics in docs#15055

Closed
spuun wants to merge 7 commits intocrystal-lang:masterfrom
spuun:include-generics-in-docs
Closed

Include generics in docs#15055
spuun wants to merge 7 commits intocrystal-lang:masterfrom
spuun:include-generics-in-docs

Conversation

@spuun
Copy link
Contributor

@spuun spuun commented Oct 3, 2024

Fixes #15053

@straight-shoota straight-shoota added this to the 1.14.0 milestone Oct 3, 2024
@Blacksmoke16 Blacksmoke16 added kind:bug A bug in the code. Does not apply to documentation, specs, etc. topic:tools:docs-generator labels Oct 3, 2024
@straight-shoota straight-shoota added the kind:regression Something that used to correctly work but no longer works label Oct 3, 2024
Copy link
Member

@straight-shoota straight-shoota left a comment

Choose a reason for hiding this comment

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

Unfortunately, this still isn't working correctly.

With this change, each generic instantiation is treated as a different including type.

For example, for Comparable:

Direct including types

Array(String) Array(Crystal) Array(Log::Metadata::Value) Array(JSON::Any) Array(LLVM::BasicBlock) Array(YAML::Any) Array(Crystal::DWARF::Abbrev::Attribute) Array(Crystal::DWARF::Abbrev) Array(Crystal::DWARF::LineNumbers::Sequence::FileEntry) Array(Crystal::DWARF::LineNumbers::Row) Array(Array(Crystal::DWARF::LineNumbers::Row)) Array({UInt64, UInt64, String}) Array(Pointer(Void)) Array(Int32) Array(IO) Array({Int32, Int32, String::Grapheme::Property}) Array(Time::Location::Zone) Array({Int32, Int32, Int32}) Array({Int32, Int32}) Array({Int32, Int32, UInt8}) Array({Int32, Int32, Unicode::QuickCheckResult}) Array(Compress::Zip::File::Entry) Array(LLVM::ABI::ArgType) Array(LLVM::Value) Array(Spec::Result) Array(Int32 | String) Array(Benchmark::IPS::Entry) Array(Pointer(Bool)) Array(UInt8) Array(Crystal::ELF::SectionHeader) Array(Int32, Exception? -> Nil) Array( -> Nil) Array(PrettyPrint::Group) Array(Array(PrettyPrint::Group)) Array(Time::Location::ZoneTransition) Array(Compress::Zip::Writer::Entry) Array(Log::Builder::Binding) Array(Log::Entry) Array(HTTP::Request -> Nil) Array(Socket::Server) Array(JSON::Builder::ArrayState | JSON::Builder::DocumentEndState | JSON::Builder::DocumentStartState | JSON::Builder::ObjectState | JSON::Builder::StartState)

@straight-shoota straight-shoota removed this from the 1.14.0 milestone Oct 3, 2024
@spuun
Copy link
Contributor Author

spuun commented Oct 4, 2024

Right. Still #must_include? that needs to be fixed. I understand what needs to be done, just haven't found out how yet. A lot to code to process.

@spuun
Copy link
Contributor Author

spuun commented Oct 7, 2024

What I do now is

  def must_include?(type : GenericInstanceType)
    return false unless type.unbound?
    type = type(type.generic_type)
    must_include? type
  end

which I'd say is "correct". One problem with this is GenericModuleInstanceType#unbound? which returns false for Comparable(Array(T)). I get that Comparable's type parameter is bound to Array(T), but since Array(T) is unbound shouldn't Comparable(Array(T)) also be that?

GenericModuleInstanceType#unbound?seems to look at its type parameters to do a "deep" unbound check , but Array(T) is a GenericClassType and GenericClassType#unbound? always return false, thus is Comparable(Array(T)) not unbound.

If I modify GenericModuleInstanceType#unbound? from to return true if type_var.type.unbound? to return true if type_var.type.unbound? || type_var.type.is_a?(GenericType) things looks much better. I dunno if it would break anything, but making GenericType#unbound? always return true would be another way of achieving the same thing.

However, e.g. BigDecimal loses all if it's including modules with this #must_include? overload, and I guess that's because it's including different bound versions of Comparable(T). So, sometimes we want the bound type and sometimes we don't. I guess I'll have to do smarter filtering in Crystal::Doc::Type#included_modules, Crystal::Doc::Type#including_modules and Crystal::Doc::Type#ancestors.

@spuun
Copy link
Contributor Author

spuun commented Oct 7, 2024

I think todays docs are missing some types e.g. NoReturn. Now when i make docs with this branch i get NoReturn in the menu, and it seems reasonable?

image

@straight-shoota
Copy link
Member

That was an unrelated change. #14817 added documentation for NoReturn and Void.

@spuun
Copy link
Contributor Author

spuun commented Oct 7, 2024

That was an unrelated change. #14817 added documentation for NoReturn and Void.

Oh 🤦


unless ast_node?
@type.ancestors.each do |ancestor|
next if ancestor.module?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this is it's own PR, but I noticed that included modules ended up in ancestors in index.json.

@straight-shoota
Copy link
Member

straight-shoota commented Oct 7, 2024

Closing because #14910 was reverted in #15064

😢

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kind:bug A bug in the code. Does not apply to documentation, specs, etc. kind:regression Something that used to correctly work but no longer works topic:tools:docs-generator

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Missing inherited methods sections

4 participants