new website implementation

Following #55 and discussions in the steering group, I have implemented a new website based on github pages, starting from the cayman theme. You can check out  the new implementation on https://robertoostenveld.github.io/bids-website/ and the technical details on the corresponding github repository.
The overall goal is to

make it easier for people to contribute (markdown instead of html)
improve and extend the content of the website
improve the structure, especially when more content will be added (hence multiple pages)
improve the formatting and accessibility of the website (fonts, colors, etc.)
consolidate documentation that is now elsewhere (RTD and Google docs)

This pull request only addresses (1) and (3). I explicitly tried to keep the content and format similar to the old/existing website. I hope that by merging this PR we can facilitate further improvements on the website.

I realize that we have more active contributors to the specification itself than to the website. Since  the website affects us all, I hope that we can get feedback from @bids-standard/everyone.

I like it so far!

FWIW I like it too.
While at it

might want to expand acknowledgements.  I believe that various developments within BIDS were supported also by various grants worth mentioning (ping @poldrack, @oesteban)
I like those brains, but now there is a nice logo - may be it should replace (or somehow complement) them.

yes, would be great to acknowledge the Laura and John Arnold Foundation,
NIMH Grant R24MH114705, and NSF grant OAC-1760950
thx!
rp
On Sat, Dec 14, 2019 at 10:15 AM Yaroslav Halchenko <
<EMAIL_ADDRESS>wrote:

FWIW I like it too.
While at it

might want to expand acknowledgements. I believe that various
developments within BIDS were supported also by various grants worth
mentioning (ping @poldrack https://github.com/poldrack, @oesteban
https://github.com/oesteban)
I like those brains, but now there is a nice logo - may be it should
replace (or somehow complement) them.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/bids-standard/bids-website/pull/56?email_source=notifications&email_token=AAGUVEGKZY2SWY2MSCORDLLQYUPFPA5CNFSM4J22QT4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEG4ICIA#issuecomment-565739808,
or unsubscribe
https://github.com/notifications/unsubscribe-auth/AAGUVEHEM7S3QYJYQVV5JP3QYUPFPANCNFSM4J22QT4A
.

--
Russell A. Poldrack
Albert Ray Lang Professor of Psychology
Professor (by courtesy) of Computer Science
Bldg. 420, Jordan Hall
Stanford University
Stanford, CA 94305
<EMAIL_ADDRESS>http://www.poldracklab.org/

As far as addressing 1 and 3 specifically, looks great.

I think it improves the website with respect to your points 1 and 3 @robertoostenveld ... looking good!

I like those brains, but now there is a nice logo - may be it should replace (or somehow complement) them.

I like the brains as well, and think having the BIDS logo as a thumbnail might be enough.

I have other comments, but I think they're out of the scope of those two points!

@adelavega perhaps you can add your points to the discussion thread on discourse, it might be useful for the discussion to have the comments before PRs are opened.

wow, thx Robert!

under benefits --> other tools --> add bids-matlab (Matlab library to harmonize access and manipulation)
when clicking on spec, why not directly link to https://bids-specification.readthedocs.io/en/stable/

@franklin-feingold @KirstieJane I believe you both have permissions to merge in this repository. I do not have myself. I think that - given the feedback so far - you should go ahead and merge my proposed changes. Or...
If you agree, you (or @sappelhoff) could also grant me the rights to do it myself. I think that would  be better, because there might be differences/issues with building and releasing the website on this account (as compared on my account) which I could fix faster if I also do the merge myself.

Oooooooh! I do have permissions to merge! 🙀
I think this looks great. I'm going to very quickly suggest a change to the contributing page but otherwise I think we're ready to merge!

I'm :+1: for merge and for giving @robertoostenveld merge permissions here.
I also agree with separating PRs for changes in form from changes in content.

Ah - sorry - I disregarded the point about separate PRs for separate points 😩 Feel free to reject my suggestions if you'd prefer to have a broader discussion about them.
I've made @robertoostenveld admin of this repository (documented in #59). Robert - thank you so much for doing this work - merge when you're ready 🚀

This is great-- thanks @robertoostenveld !
Small thing that could be in another PR: On small screens (like mobile) it's hard to tell that any of the sub-pages have launched since the nav takes up the whole screen. Would be great to have these collapse into a (sticky?) navbar on small screens. If that's of interest for this PR I can try to see where you have it defined in the code, here.

This is great-- thanks @robertoostenveld !
Small thing that could be in another PR: On small screens (like mobile) it's hard to tell that any of the sub-pages have launched since the nav takes up the whole screen. Would be great to have these collapse into a (sticky?) navbar on small screens. If that's of interest for this PR I can try to see where you have it defined in the code, here.

If this is quickly doable, I think it would be good to try to get in this PR.

Nice catch @emdupre. I agree this should be taken care of before merging.
Also, something that wasn't so obvious on desktop but is on mobile is that the main title is not a link back to the "home" page (in this case "about"). I feel like this is a pretty common convention that is nice to follow.
Relatedly, that makes me realize that the main header is text instead of the new BIDS logo that was recently adopted (and is used on the current website, which I hadn't realized). It would make a lot of sense to highlight this branding, and it conveniently also makes for a better clickable "home" button.
I feel less strongly about this last part, but it might also make sense to get rid of the "about" section/button altogether, and have the information that is there be the main "home" landing screen, that be accessed by clicking on the BIDS logo/header.
(This includes some of the feedback I was holding back on, sorry!)

As an outsider I just wanted to drop in and say that this layout is much cleaner, thanks @robertoostenveld ! However, I wonder if hitting specification should automatically direct you to the specification rather than being an additional click away. That may be out of the scope of this PR, though, since that's hosted on RTD rather than directly on the .io page.

@jbteves I had similar thoughts, maybe they can wait until a PR having to do more with content though.
Having an external link in the main nav is not ideal because it can catch users off guard (unless it has an icon to indicate its an external link). An alternative would be to have more info on the Specification page, so that it serves another purpose and doesn't feel too empty.

We can move this to an issue if you like. I feel that the specification would ideally live in the same place as the main website; maybe they could coexist on RTD?

I think what's hard about this is that this PR is trying to address a small (but important !) fraction of the feedback folks have around the website.
It sounds like its rendering on mobile might be in scope of this PR, though. Can you confirm @robertoostenveld ? It will be some work on my part so I don't want to start in the wrong place.

It looks like there's two main issues here (that IMO are relevant to this PR, but could also be addressed by someone else in another incremental PR).

As a consequence of moving away from a one page set up, some pages seem a bit empty now that they are on their own (e.g "specification" and "acknowledgements" pages). Some may never warrant that much content, and it seems a bit odd to me to have them on their own page ("acknowledgements")
The menu is too long on mobile

From a quick search, it seems adding a hamburger menu might require a custom implementation, as it's not something mentioned on the Cayman theme.
I wonder if an easy fix here is to remove 1-2 sections. Maybe removing the "About" button, and having it be "Home" (reachable by clicking the header/BIDS Logo). We could also remove "Acknowledgements", and append it to this "Home" page.  That might free up enough space on mobile to see the content has changed.
In any case the main contribution of this PR I think was to port to Jekyll/Markdown (which is awesome! thank you), and most of the issues we're having here are more nitpicky about the navigation/multi-page layout.

Hmm. Looks like there's a relative path issue:

<link rel="stylesheet" href="/bids-website/assets/css/style.css?v=c4c24382182f2b2a1a7205adbed757e7048e760f">

That bids-website/ is invalid with the custom domain...

yes… working on it

On 19 Dec 2019, at 22:19, Chris Markiewicz<EMAIL_ADDRESS>wrote:
Hmm. Looks like there's a relative path issue:

That bids-website/ is invalid with the custom domain...
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub https://github.com/bids-standard/bids-website/pull/56?email_source=notifications&email_token=AAG3PY7SQDI326IIZAKBHTLQZPQNRA5CNFSM4J22QT4KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEHLCAFA#issuecomment-567681044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAG3PY24UW5EFYJH7HCRAHLQZPQNRANCNFSM4J22QT4A.

Thanks for all the feedback and suggestions. The new site is now deployed, including a menu that folds in on small/mobile screens.
It was a bit of a struggle to get the website working with the proper variable replacement in the code on the github pages hosting service, while using the bids.neuroimaging.io CNAME. It now works, although the URL for the background image in the header is now hardcoded. Good enough for now...
I will copy all remaining suggestions to separate issues and CC you on them. These can now be followed up in the regular way, i.e. preferably as a PR.


Optimise XPath queries

There's quite a few queries in LWD which do things like:
    text_field = find_field(world.browser, 'text', field_name) or \
        find_field(world.browser, 'textarea', field_name) or \
        find_field(world.browser, 'password', field_name) or \
        ...

Each one of these triggers a single XPath query to the WebDriver, which if you're using a network webdriver, such as BrowserStack, is another round-trip across the Internet.
Need to combine these into single queries to the WebDriver.

This has been fixed in #42.

@koterpillar Why fixed issues are remaining open?

@miratcan I am not the maintainer for this repository and cannot close it.


Launchers to take kwarg for job file directory

Specify where these files go, otherwise, it's a mess quickly
https://github.com/Parsl/parsl/blob/6c77c3f198981e598c9848c99b6852bcbeba923f/parsl/launchers/launchers.py#L325

In a shared filesystem setup, this might make sense to default to the rundir (though this would be awkward to wire through with the way launchers are implemented).
Maybe all of the launchers which create intermediate files assume a shared filesystem anyway.


Bring unknown ping worlds to bottom of world list when sorting in ascending

Right now you need to scroll down and either click on the quickly rising world with lowest ping or wait for all the worlds to be pinged.

I was thinking about this yesterday; I think a better solution is to postpone resorting of the list until after all worlds have updated or to not clear out a world's ping until the new ping is found. I'm not very comfortable in async programming and having trouble implementing either solution, however.

I've done this as part of https://github.com/runelite/runelite/pull/9020.

put Closes #8176 in your pr's description, rather than linking to it here


isNode returns false for electron apps

The current implementation for isNode is:
export const isNode = typeof navigator === "undefined" && typeof process !== "undefined";

For electron apps this will return false and I believe we need it to return true.
Suggested edit:
export const isNode = !!process && !!process.version && !!process.versions && !!process.versions.node;

cc @bterlson

!!process.version is this part necessary?


adding seeeduino wio terminal for Circuit Python usb driver

This allows the https://circuitpython.org/board/seeeduino_wio_terminal/  to be recognized by Windows 7 so that Serial Console and REPL can be used.
I do have a device and tested this successfully
I am not affiliated with Seeed. I was able to lookup the devices VID / PID in device manager.

I am adding a bunch of new Adafruit boards and will add something functionally equivalent to this at the same time.


add webcam to cameras

vipc refactor will happen in a separate PR later

TODO: wait for sensorValid fix, no hacks

Looks like you need to bump cereal to get tests to pass?

Can we merge this before tici camera work?

Can we merge this before tici camera work?

waiting for tests


Chrome's FileSystem API has a bug reading files from dropped folders or input dialog's selected folder

Chrome 83.0.4103.61, Windows 10.0.18362 Chrome's FileSystem API has a bug that files from dropped folders or files from input dialog's selected folder, with read errors (has absolute paths which exceed 260 chars) will have zero file size.
When dropped folder has any file with absolute paths which exceeds 260 chars, upload should report read errors.
When input dialog is used to select folder, files with absolute paths which exceeds 260 chars, will be uploaded with zero length. These files should not be uploaded, it should report read errors.
Firefox 76.0.1, Windows 10.0.18362 doesn't have this bug, but it will not upload files with absolute path which exceeds 260 chars.
Edge 44.18362.449.0, Windows 10.0.18362 uploads all files regardless of the absolute path length.

It's unlikely such a huge diff (+149 −52) could be efficiently reviewed. Could you provide a minimum testcase and a less intrusive bugfix?


Allow modules and classes to specify required ancestors

Motivation
(This coming from the documentation included in this pull-request)
It's not uncommon in Ruby to define helper modules that depends on other modules.
For example, let's take the following helper which provides say_error method:
# typed: true

module MyHelper
  def say_error(message):
    raise "InternalError: #{message}" # error: Method `raise` does not exist on `MyHelper`
  end
end

class MyClass
  include MyHelper

  def do_something(x)
    say_error("some error") unless x
    # ...
  end
end

If we run Sorbet on this example, we will get a type-checking error saying that the method raise
does not exist on MyHelper since this method is defined on Kernel.
Thanks to this error, Sorbet is protecting us against some edge-cases where we would try to include
the MyHelper module in a class that does not include Kernel:
class MyBaseClass < BasicObject
  include MyHelper

  def do_something(x)
    say_error("some error") unless x
    # ...
  end
end

MyBaseClass.new.do_something(false) # runtime-error: in `say_error': undefined method `raise' for #<MyBaseClass> (NoMethodError)

This example would raise an error at runtime because the method raise is undefined for instances of MyBaseClass
as it doesn't include Kernel.
Requiring Ancestors
Sorbet provides the requires_ancestor method as a way to ensure that classes or modules including MyHelper
will also include Kernel.
Let's change our base example to use requires_ancestor:
module MyHelper
  extend T::Helpers

  requires_ancestor Kernel

  def say_error(message):
    raise "InternalError: #{message}"
  end
end

This way we specify that any module including MyHelper must also include Kernel
and Sorbet will display an error if it's not the case:
class MyBaseClass < BasicObject # error: `MyBaseClass` must include `Kernel` (required by `MyHelper`)
  include MyHelper
end

requires_ancestor also works to require that a specific class must be inherited:
module MyHelper
  extend T::Helpers

  requires_ancestor Object

  def class_name
    self.class.name
  end
end

class MyBaseClass < BasicObject # error: `MyBaseClass` must inherit `Object` (required by `MyHelper`)
  include MyHelper
end

Note that requirements are transitive:
class MyBaseClass2 < MyBaseClass # error: `MyBaseClass2` must inherit `Object` (required by `MyHelper`)
  include MyHelper
end

requires_ancestor can be used to require more than one ancestor:
module Test
  module TestAssertions
    def assert_equal(x, y)
      x == y
    end
  end

  class TestBase
    def test; end
  end

  class TestCase < TestBase
    include TestAssertions
  end
end

module MyLogger
  def log_test_failed; end
end

module MyTestHelper
  extend T::Helpers

  requires_ancestor Test::TestAssertions, MyLogger

  def assert_not_equal(x, y)
    if assert_equal(x, y)
      true
    else
      log_test_failed
      false
    end
  end
end

class MyValidTest < Test::TestCase
  include MyTestHelper
  include MyLogger
end

class MyBrokenTest < Test::TestBase # error: `MyBrokenTest` must include `Test::TestAssertions` (required by `MyTestHelper`)
                                    # error: `MyBrokenTest` must include `MyLogger` (required by `MyTestHelper`)
  include MyTestHelper
end

How it works?

A resolver job collects the required ancestors specified with requires_ancestor X, Y and stores them in a magic <required-ancestor> property.

The symbol can read the list of directly required ancestors from this magic <required-ancestor> property. Or compute recursively the list of all its ancestors on demand.

The definition validator will use both these lists to ensure all non-abstract classes do include all the ancestors their own includes require (and a few other sanity checks).

During calls validation, if a method called is not found, we also look into the required ancestors to see if the method exists before raising an error.

But at what cost?
None if you don't use any requires_ancestor in your code.
If you do use them, you'll pay the price only where used:

Cost some memory to store the magic "" property only for classes/modules that do use it.
If a method is not found on a receiver, cost a bit to recursively look into all ancestors of the receiver if one of the required ancestors provides the method. So you pay only for NoMethod errors if you don't actually use the feature.

Reviews
This pull-request can be read commit by commit.
Test plan

See included automated tests for more examples on how it works and the kind of errors it produces.
This should also provide an answer to https://github.com/sorbet/sorbet/issues/2236.

I've really wanted something like this! Not qualified to comment on the implementation, but a question about naming - this feels closely related to the bind method on T.proc, in that it's basically an assertion about the type of self. Should it have bind in the name somehow, like bind_self? I can't think of anything that's quite as clear as required_ancestor, though...

@djudd-stripe

in that it's basically an assertion about the type of self. Should it have bind in the name somehow, like bind_self?

Good point. My feelings against bind is that it doesn't give the user any clue on how to solve the problem. While requires_ancestor at least points to the including/subclassing mechanism.
I'm not opposed to change the name if you come with something better.

I looked at the failing error for LSP. It seems that the errors are not reported again after the fast path.
I added a few prints to understand what is happening and the check is actually called and does the right thing. It's beginError that doesn't seem to display the error. Is it expected?
Should I disable the fast path for those tests or is there a way to force beginError to  create the error a second time?

I looked at the failing error for LSP. It seems that the errors are not reported again after the fast path.
I added a few prints to understand what is happening and the check is actually called and does the right thing. It's beginError that doesn't seem to display the error. Is it expected?
Should I disable the fast path for those tests or is there a way to force beginError to create the error a second time?

I think this indicates a somewhat fundamental limitation with the design. The fast path by design only runs the runTreePasses phase of resolver. This excludes most of the GlobalPass checks. This is part of what makes the fast path fast: it only re-runs Sorbet on the parts that it thinks could have changed, and all the GlobalPass logic is slow.

The whole thing is heavily inspired from the validator for abstract (you can see it with the caching mechanism actually). At first I put the checks in the definition_validator but I also need to change the GlobalPass to perform the linearization of the required ancestors (so method resolution works). So I moved everything to the class in the GlobalPass to minimize the diffs.

I think what you want to do is move as much of your logic back to definition_validator, which is run per changed file, not for all files. When the fast path runs, the definition_validator checks will always run a second time, so the errors that are currently not showing up will start to show up.
I think that by moving the logic to definition_validator, you will also be forced to stop using the ancestorsCache that you're currently using. What was your goal with that? It's called linearizeRequiredAncestors but it doesn't seem to be doing something similar to what computeLinearization is doing.

I've really wanted something like this! Not qualified to comment on the implementation, but a question about naming - this feels closely related to the bind method on T.proc, in that it's basically an assertion about the type of self. Should it have bind in the name somehow, like bind_self? I can't think of anything that's quite as clear as required_ancestor, though...

@djudd-stripe When I see bind I think "this is some lurky instance_eval / instance_exec hack" and that's not what's going on at all. When we were discussing this feature internally, we always described it by comparison with abstract classes. The idea was that if you're using this feature, the class or module is essentially abstract! until it's required ancestors are met.
I don't have strong thoughts on the name. I think the API will always be something akin to some_method_name SomeModule which is super easy to write an autocorrect for later if we want to change the name.

Ok another follow up:
I chatted with @elliottt about this for about an hour trying to make sense of how this change works / should work.
I think we're both on the same page as far as moving as many of the check*Ancestors helpers into definition_validator, which should fix the LSP fast path test failures you're seeing.
Two other things that jumped out to us as out of place:

The change to findMemberTransitiveInternal seems dangerous. It surprised us to see that code changing. Another change which I think has the same effect: there's a line in builder_entry.cc that ascribes the type of the synthetic <self> variable:
https://github.com/sorbet/sorbet/blob/master/cfg/builder/builder_entry.cc#L34-L37
I think for methods whose owning class has a non-empty requiredAncestors() list, we can change this Cast instruction to ascribe an intersection type, like T.all(MyOwner, T.all(... all my required ancestors ...)). So for a concrete example it might look like <self> = T.cast(<self>, T.all(MyModule, Kernel)). The intersection type would then take care of looking for members in different places, and we don't have to worry about changing a really low level primitive.

The implementation of linearizeRequiredAncestors looks like it's doing something really similar to computeLinearization. computeLinearization already deals with the superClass and the mixins vectors on a Symbol, and it seems like requiredAncestors is similar in spirit to those. In particular, the way that method is written effectively uses the symbol table itself as a cache. There's an isClassOrModuleLinearizationComputed flag on every ClassOrModule symbol that says whether linearization was computed or not. That's kind of like your alreadySeen vector, and the symbol table itself is the ancestorsCache.
Do you think you could try moving the logic to linearize required ancestors into computeLinearization? For example, this function has kind of two parts:
https://github.com/sorbet/sorbet/blob/17bd9ff63dfb471574060acaf3e068e756f88cf9/resolver/GlobalPass.cc#L275-L326
There's a short part at the top that handles the superClass() case. Then there's a longer part that handles the mixins() case. And I think we could add a third one that handles the requiredAncestors case.
A couple benefits of this:

computeLinearization is already called from the resolver::runTreePasses function. Given the current state of the change, I think that we will incorrectly take the fast path when we should take the slow path, because the linearizeRequiredAncestors function is not called on the fast path. The way you can test this is to write an # assert-slow-path: true test (grep for some examples), like this:

# -- test/testdata/lsp/foo.rb --
# typed: true
# assert-slow-path

module A
  def foo_a; end
end
module B
  # requires_ancestor A
  def foo_b
    foo_a
  end
end

# -- test/testdata/lsp/foo.1.rbupdate --
# typed: true
# assert-slow-path

module A
  def foo_a; end
end
module B
  requires_ancestor A
  def foo_b
    foo_a
  end
end

and this test will assert that the slow path must have been taken. This is important, because the symbol table changed, which means we have to type check everything again.

by sharing the traversal of GlobalState with the mixins and superClass in computeLinearization, it should have better cache locality, and should use less memory because we're re-using the symbol table to store things, instead of making ancestorsCache, which will be a huge helper data structure.

Sorry for the massive comments here. If you want to video chat some time, please don't hesitate to ask. I'm free almost any time from 10:00 a.m. – 6:00 p.m. PT.

I pushed a few changes that make everything work with both slow/fast path and I will try to address all the comments here.
First, I changed the implementation in several ways

In the symbol, we only store the location of the locally required ancestors (vs the location of all linearized ancestors before)
The error reporting traverses the ancestors recursively to find the first one which introduced the ancestor locally and get the location (a bit slower here because we can't rely on the linearization and have to traverse the tree)
I moved all the checks to the validator pass
What was missing was clearing the local ancestor list between reload thus ending in the same ancestor being required and added again to the symbol at reload

About the feature, I imagine the requires_ancestor X as two properties:

it does behave like include X to resolve the methods (methods from X are available from self)
it does not behave like include X for subtyping (self is not a subtype of X)

What I understand from the two solutions you are proposing would mean considering X as a parent of self.
My approach with this PR is the following:

In the resolver, when the file is (re)loaded, we clear the list of local ancestors and load all of them from the tree (they might have beed edited since the last time)
In the global pass, once everything is loaded we (re)compute the linearization of the ancestors for everything (since an ancestor might have been introduced somewhere we have to invalidate everything and start over).
In the validator. For each ancestor X in the linearized list of self (so all local + transitive ones but already linearized, not recursive) we check if self subtypes X (so the requirement has been satisfied somewhere in the inheritance tree).
In the Symbol when we're looking for a method, if it cannot be found anywhere, we look into the linearized list of ancestors (it's not recursive, it calls findMember). What I think is missing here is a case for when the linearization of the ancestors has not been computed yet, but I'm not sure how to test that.

I think the linearization of the ancestors must be based on the linearization of the mixins (and thus done after it) for multiple reasons:

It's faster since all the mixins have been resolved for self we don't need to recurse for them. We only need to do it for the superclasses (unless we have that linearization somewhere too??) and for the required ancestors (because the requirements are transitive and we are building the said linearization for them).
It allows us to break cycles and reflexive requirements. I think this is a necessary feature for the sake of the user + we can easily do it there.
It's less intrusive, and avoid changing complex parts.

@jez regarding your example I added it to this PR to show that it works.

https://sorbet.run/#%23 typed%3A true
class MyAbstractModel
  def query(query_string)%3B end
end

module ModelMixin
  requires_ancestor MyAbstractModel
  %23 include MyAbstractModel

  %23 ancestors %3D [...%2C <StartOfRequiredAncestors>%2C ...]

  sig {params(...).returns()}
  def <required_ancestors>
  end

  def mixin_method
    this %3D T.cast(self%2C T.all(ModelMixin%2C MyModel))%0A%20%20%20%20this%20%3D%20self%0A%20%20%20%20this.query('foo')%20%20%20%20%20%20%20%20%20%20%20%20%23%20why%20allow%20this%20but...%0A%20%20%20%20ModelHelpers.query_foo(this)%20%23%20...%20disallow%20this%3F%0A%0A%20%20%20%20super%0A%20%20end%0Aend%0A%0Aclass%20ModelHelpers%0A%20%20extend%20T%3A%3ASig%0A%0A%20%20sig%20%7Bparams(model_instance%3A%20MyAbstractModel).void%7D%0A%20%20def%20self.query_foo(model_instance)%0A%20%20%20%20model_instance.query('foo')%0A%20%20end%0Aend%0A%0Aclass%20MyModel%20%3C%20MyAbstractModel%0A%20%20include%20ModelMixin%0Aend%0A%0AMyModel.new.mixin_method
https://sorbet.run/#%23 typed%3A true
class Module
  include T%3A%3ASig
end

%23 -- foo.rb --
module Parent
  extend T%3A%3AHelpers
  %23 (1) Mark module as sealed
  sealed!
end

%23 (2) CAN be included%2Fextended%2Finherited in `foo.rb`
class Child1%3B include Parent%3B end
class Child2%3B include Parent%3B end

sig {params(x%3A Parent).void}
def foo(x)
  case x
  when Child1 then %23 ...
  when Child2 then %23 ...
  %23 (3) Sealed classes support exhaustiveness
  else T.absurd(x)
  end
end

%23 -- bar.rb --
%23 (4) CANNOT be included%2Fextended%2Finherited in other files
%23 class BadChild%3B include Parent%3B end %23 error!
https://sorbet.run/#%23 typed%3A strict
module MyAbstractModel
  def query(query_string)%3B end
end

module ModelMixin1
  include MyAbstractModel %23 isClassOrModuleModule

  def mixin_method1
  end
end

module ModelMixin2
  include MyAbstractModel

  def mixin_method2
  end
end

class MyModel
  include ModelMixin1
  include ModelMixin2
  include MyAbstractModel
end
https://sorbet.run/#%23 typed%3A true
class A%3B end
module M
  include A
end

We have a similar problem to #2236. Our app has objects that have hundreds of calculated values in methods. Since there's so many, we've broken them out into separate modules for organization. Each class without separate modules would be thousands of lines long.
class SomeObject
  include SomeObjectCalc1
  include SomeObjectCalc2

  def some_object_method
  end
end

module SomeObjectCalc1
  def do_math
    some_object_method * 50
  end
end

module SomeObjectCalc2
  def do_another_math
    some_object_method * 100
  end
end

In this case, our required "ancestor" is the class it will be included in.
Will this PR support this scenario? There doesn't seem to be any other way to model this in Sorbet and I've been watching for something like this PR (thanks!)

@jez new version of this PR 🎉
First, I totally changed the approach so it does not require changes to the method resolution mechanism. Instead, we wait for callDispatch to not find a method called and check if a required ancestor provides it and if it does, we dispatch the call to this one. So we pay the price only in cases where Sorbet would raise a NoMethod error. The cost being looking if the superclasses and the linearized list of included modules (already computed done by Sorbet earlier) have required ancestors.

If no required ancestor is found for the receiver type, dispatchCall raises the expected NoMethod error.
If there are transitively required ancestors for the receiver type, we try to dispatch the call to each of them until the first one that accepts it, or raise the NoMethod error if none can.

Then, I also changed the way the data are stored to use a fake method definition like it's done for sealed classes. Ancestors are stored in the return type (a TupleType) and the associated locations in the method symbol locs so the memory footprint is 0 if you don't use the feature.
Finally, to answer the remaining questions in this thread:

Can we reuse the module linearization process?
No since we allow multiple required ancestors and ancestors to be (abstract) classes, so it wouldn't fit inside the current module linearization process. Also, no since we allow requirement cycles. And finally, no since creating a requirement on an ancestor does not mean creating a subtyping relationship between the requiree and the required. It only creates a constraint on the thing including / subclassing the requiree.
Note that we still reuse the module linearization as it was computed in the GlobalPass when going up the tree so this part is not duplicated.

What about sorbet-runtime?
Currently, requires_ancestor just no-ops at runtime. So the runtime works as before and will raise the same error as before is something required should have been included and is not.

Is it supported in LSP mode?
Yes, see the test under test/testdata/lsp and also not the the POS tests do not diable the fast path.

@elliottt I handled you comments and did a few improvements:

The transitive list of ancestors is computed ahead of time during the global pass and saved inside a magic <required-ancestors-lin> property which stores the list of required ancestors for a symbol, the origin or the requirement, and the loc of the requirement.
No more res passed around, the ancestors are directly recorded inside the <required-ancestors-lin> property.

Rebased on master

Rebased on master

Here's the example we talked about today @Morriar:

# typed: true
module A
  def bar(x); end
end
module B
  def bar; end
end
module C
  requires_ancestor A
  requires_ancestor B
  def foo
    bar
  end
end
class Parent
  include B
  include A
end
class Child < Parent
  include C
  def main
    foo
  end
end
Child.new.main

Here's the example we talked about today @Morriar:

# typed: true
module A
  def bar(x); end
end
module B
  def bar; end
end
module C
  requires_ancestor A
  requires_ancestor B
  def foo
    bar
  end
end
class Parent
  include B
  include A
end
class Child < Parent
  include C
  def main
    foo
  end
end
Child.new.main

I protected the feature behind a --enable-experimental-requires-ancestor flag

I protected the feature behind a --enable-experimental-requires-ancestor flag

I asked this before, but I wanted to give a specific use case to see if this will be supported by this branch. We have hundreds of extra methods for our models and have organized them into related modules. We need to be able to call into other modules and the class' instance methods, as it's only for organizational purposes.
class SomeModelClass
  include SomeModelClassExtension
  include SomeModelClassExtension2
  include SomeModelClassExtension3
  include SomeModelClassExtension4
  include ...

  def project
  end
end

module SomeModelClassExtension
  requires_ancestor SomeModelClass # ???
  include_types SomeModelClass # ???

  def some_activity
    project # Sorbet currently does not seem to be able to type this
  end
end

Thanks!

I asked this before, but I wanted to give a specific use case to see if this will be supported by this branch. We have hundreds of extra methods for our models and have organized them into related modules. We need to be able to call into other modules and the class' instance methods, as it's only for organizational purposes.
class SomeModelClass
  include SomeModelClassExtension
  include SomeModelClassExtension2
  include SomeModelClassExtension3
  include SomeModelClassExtension4
  include ...

  def project
  end
end

module SomeModelClassExtension
  requires_ancestor SomeModelClass # ???
  include_types SomeModelClass # ???

  def some_activity
    project # Sorbet currently does not seem to be able to type this
  end
end

Thanks!

@elliottt I implemented all the changes you required but the one about moving the method from private to anonymous as it's breaking current approach and ends up making the code harder to read.
It's ready for another review 👍

@bbugh: yes, the requires_ancestor will ensure that only SomeModelClass  (or a subclass) can include SomeModelClassExtension  making project available.
Not sure what is the include_types about?

@Morriar Awesome, thank you! And thank you for this fantastic branch! We'll be able to type our code that would benefit most from strong types.
P.S. Sorry for the confusion about include_types, I couldn't remember the name of requires_ancestor and it looks like I forgot to delete it when I added the correct one.

@elliottt thanks for the review!

Just to make sure that your flag in the test suite is working as expected, could you add a test that uses requires_ancestor but doesn't enable the feature?

Done, see test/testdata/resolver/requires_ancestor_disabled.rb

If you wouldn't mind rebasing this on master, I'll test it on the stripe codebase to make sure that we don't see a performance regression.

And done and fixed the conflicts.

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR.  When the build
finishes, I'll share with you whether or how it failed. Thanks!
Stripe employees can see the build results here:
→ https://go/builds/bui_IvxFK1YO3JlRaF
→ https://go/builds/bui_IvxFVKOMdjbmM2

We have a policy of testing changes to Sorbet against Stripe's codebase before
merging them. I've kicked off a test run for the current PR. When the build
finishes, I'll share with you whether or how it failed. Thanks!
Stripe employees can see the build results here:
→ https://go/builds/bui_IvxFK1YO3JlRaF
→ https://go/builds/bui_IvxFVKOMdjbmM2

This was a successful build with no change in perf 🎉