[Feature #18885]
For now, the optimizations performed are:
- Run a major GC
- Compact the heap
- Promote all surviving objects to oldgen
Other optimizations may follow.
Previously, when sorting and comparing git Gemfile vs lockfile sources during
`bundler/setup` to figure out whether we need to re-resolve or not, we
would try to find the default branch if nothing more specific was
specified in the Gemfile.
If the git cache has been deleted thought, that would fail.
The error would still be swallowed (and the branch would simply not be
displayed), but trying to clone would still generate the side effect of
creating the parent folder for the clone.
That could affect non-writable systems that don't expect `bundler/setup`
to write to the filesystem at all.
To fix this, override `Bundler::Source::Git#identifier` to use
exclusively static information, so it does not even try to clone the
repo nor generate any side effects.
https://github.com/rubygems/rubygems/commit/582eb2ef39
We have some flags that limit running git commit commands under certain
situations, for example, when running under `--local`. However, those
should only affect remote git operations, not local read-only operations
like `git --version`, or `git rev-parse --abbrev-ref HEAD`.
This commit refactors things to achieve that.
By doing this, the `#to_s` representation of a source is more
consistent, since we don't get any errors when reading the checked out
branch, and we avoid some flip-flop lockfile issues.
https://github.com/rubygems/rubygems/commit/4a529fce81
When dependencies in path sources have changed, we'll be re-resolving,
and we can't really know whether the resolution will be valid or invalid
for the Ruby platform, so skip the removal in that case.
https://github.com/rubygems/rubygems/commit/afc3b0956f
This error message is also printed when using `bundler/setup` in frozen
model, so we're not necessarily installing any gems when it happens.
This new message play nicer with all situations.
https://github.com/rubygems/rubygems/commit/6874bbacce
Similarly to how the other ignored files are intended for local
development and not for production, the Gemfile and Gemfile.lock files
for a gem only relate to local development and aren't useful to people
installing the gem.
https://github.com/rubygems/rubygems/commit/59049c04be
If we're in inline mode, Bundler first resolves using only local gems,
and if some gems are missing, then it re-resolves using remote gems.
However, "source resolution" from the initial "local" try was being
memoized, resulting in Bundler not looking for some gems remotely in the
second resolution.
This commit forces a proper re-resolve in this case.
https://github.com/rubygems/rubygems/commit/fdc631075e
* Add rb_io_path and rb_io_open_descriptor.
* Use rb_io_open_descriptor to create PTY objects
* Rename FMODE_PREP -> FMODE_EXTERNAL and expose it
FMODE_PREP I believe refers to the concept of a "pre-prepared" file, but
FMODE_EXTERNAL is clearer about what the file descriptor represents and
aligns with language in the IO::Buffer module.
* Ensure that rb_io_open_descriptor closes the FD if it fails
If FMODE_EXTERNAL is not set, then it's guaranteed that Ruby will be
responsible for closing your file, eventually, if you pass it to
rb_io_open_descriptor, even if it raises an exception.
* Rename IS_EXTERNAL_FD -> RUBY_IO_EXTERNAL_P
* Expose `rb_io_closed_p`.
* Add `rb_io_mode` to get IO mode.
---------
Co-authored-by: KJ Tsanaktsidis <ktsanaktsidis@zendesk.com>
[Feature #19236]
In Ruby 3.3, `Hash.new` shall print a deprecation warning if keyword arguments
are passed instead of treating them as an implicit positional Hash.
This will allow to safely introduce a `capacity` keyword argument in 3.4
Co-authored-by: Jean Boussier <byroot@ruby-lang.org>
https://github.com/ruby/syntax_suggest/pull/187 Handle if/else with
empty/comment
line
Reported in #187 this code:
```
class Foo
def foo
if cond?
foo
else
# comment
end
end
# ...
def bar
if @recv
end_is_missing_here
end
end
```
Triggers an incorrect suggestion:
```
Unmatched keyword, missing `end' ?
1 class Foo
2 def foo
> 3 if cond?
> 5 else
8 end
16 end
```
Part of the issue is that while scanning we're using newlines to determine when to stop and pause. This is useful for determining logically smaller chunks to evaluate but in this case it causes us to pause before grabbing the "end" that is right below the newline. This problem is similar to https://github.com/ruby/syntax_suggest/pull/179.
However in the case of expanding same indentation "neighbors" I want to always grab all empty values at the end of the scan. I don't want to do that when changing indentation levels as it affects scan results.
There may be some way to normalize this behavior between the two, but the tests really don't like that change.
To fix this issue for expanding against different indentation I needed a way to first try and grab any additional newlines with the ability to rollback that guess. In #192 I experimented with decoupling scanning from the AroundBlockScan logic. It also added the ability to take a snapshot of the current scanner and rollback to prior changes.
With this ability in place now we:
- Grab extra empties before looking at the above/below line for the matching keyword/end statement
- If there's a match, grab it
- If there's no match, discard the newlines we picked up in the evaluation
That solves the issue.
https://github.com/ruby/syntax_suggest/commit/513646b912
AroundBlockScan started as a utility class that was meant to be used as a DSL for scanning and making new blocks. As logic got added to this class it became hard to reason about what exactly is being mutated when. I pulled the scanning logic out into it's own class which gives us a clean separation of concerns. This allowed me to remove a lot of accessors that aren't core to the logic provided by AroundBlockScan.
In addition to this refactor the ScanHistory class can snapshot a scan. This allows us to be more aggressive with scans in the future as we can now snapshot and rollback if it didn't turn out the way we wanted.
The change comes with a minor perf impact:
before: 5.092678 0.104299 5.196977 ( 5.226494)
after: 5.128536 0.099871 5.228407 ( 5.249542)
This represents a 0.996x change in speed (where 1x would be no change and 2x would be twice as fast). This is a 0.38% decrease in performance which is negligible. It's likely coming from the extra blocks being created while scanning. This is negligible and if the history feature works well we might be able to make better block decisions which is means fewer calls to ripper which is the biggest bottleneck.
While this doesn't fix https://github.com/ruby/syntax_suggest/issues/187 it's a good intermediate step that will hopefully make working on that issue easier.
https://github.com/ruby/syntax_suggest/commit/ad8487d8aa
I previously left a comment stating I didn't know why a certain method existed. In investigating the code in `CaptureCodeContext#capture_before_after_kws` I found that it was added as to give a slightly less noisy output.
The docs for AroundBlockScan#capture_neighbor_context only describe keywords as being a primary concern. I modified that code to only include lines that are keywords or ends. This reduces the output noise even more.
This allows me to remove that `start_at_next_line` method.
One weird side effect of the prior logic is it would cause this code to produce this output:
```
class OH
def hello
def hai
end
end
```
```
1 class OH
> 2 def hello
4 def hai
5 end
6 end
```
But this code to produce this output:
```
class OH
def hello
def hai
end
end
```
```
1 class OH
> 2 def hello
4 end
5 end
```
Note the missing `def hai`. The only difference between them is that space.
With this change, they're now both consistent.
https://github.com/ruby/syntax_suggest/commit/4a54767a3e
`GETRUSAGE_BASED_CLOCK_PROCESS_CPUTIME_ID` clock uses `getrusage`
always if available as the name states. That is if it is implemented
`getrusage` is available, regardless microseconds in its results.
Prior to commit 5806c54447, it was "at
least one result with precision beyond milliseconds (with none-zero
microseconds) should exist"; after this commit, "at least one result
should have zero microseconds". This chance is lower than the
previous condition.
Since these files are written in a wide character encoding, stop at
first NUL byte and are actually empty. ASCII-incompatible encodings
have never been supported as source encoding.
If Gemfile has a lot of dependencies, we have an optimization that uses
the full index in that case, assuming it's going to be faster.
I think this is an old optimization that predates compact index API
times, I believe we no longer need it these days.
Also, since a few releases ago we check for circular dependencies when
resolving by looping through all versions of each name and removing
those that have circular dependencies that would trip up the resolver.
This loop becomes actually very slow when full indexes are used because
to find dependencies of a gemspec, we need to explicitly fetch the
marshaled gemspec (`gemspec.rz` endpoint) for it, so the optimization
has the opposite effect of making things very slow.
https://github.com/rubygems/rubygems/commit/2f46289bd3
Fix a CI error and add a test to ensure we're testing the current version:
```
Run bundle exec rake test
bundler: failed to load command: rake (/home/runner/work/syntax_suggest/syntax_suggest/vendor/bundle/ruby/3.2.0/bin/rake)
/opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/runtime.rb:309:in `check_for_activated_spec!': You have already activated syntax_suggest 1.0.2, but your Gemfile requires syntax_suggest 1.0.3. Since syntax_suggest is a default gem, you can either remove your dependency on it or try updating to a newer version of bundler that supports syntax_suggest as a default gem. (Gem::LoadError)
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/runtime.rb:25:in `block in setup'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/spec_set.rb:138:in `each'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/spec_set.rb:138:in `each'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/runtime.rb:24:in `map'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/runtime.rb:24:in `setup'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler.rb:151:in `setup'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/setup.rb:20:in `block in <top (required)>'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/ui/shell.rb:136:in `with_level'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/ui/shell.rb:88:in `silence'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/setup.rb:20:in `<top (required)>'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/cli/exec.rb:56:in `require_relative'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/cli/exec.rb:56:in `kernel_load'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/cli/exec.rb:23:in `run'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/cli.rb:483:in `exec'
from /opt/hostedtoolcache/Ruby/3.2.1/x64/lib/ruby/gems/3.2.0/gems/bundler-2.3.14/lib/bundler/vendor/thor/lib/thor/command.rb:27:in `run'
```
Originally I fixed https://github.com/ruby/syntax_suggest/pull/177 by making the process of comment removal indentation aware. The next commit is the more general fix and means we don't need to carry that additional logic/overhead.
Also: Update syntax via linter
While #177 is reported as being caused by a comment, the underlying behavior is a problem due to the newline that we generated (from a comment). The prior commit fixed that problem by preserving whitespace before the comment. That guarantees that a block will form there from the frontier before it will be expanded there via a "neighbors" method. Since empty lines are valid ruby code, it will be hidden and be safe.
## Problem setup
This failure mode is not fixed by the prior commit, because the indentation is 0. To provide good results, we must make the algorithm less greedy. One heuristic/signal to follow is developer added newlines. If a developer puts a newline between code, it's more likely they're unrelated. For example:
```
port = rand(1000...9999)
stub_request(:any, "localhost:#{port}")
query = Cutlass::FunctionQuery.new(
port: port
).call
expect(WebMock).to have_requested(:post, "localhost:#{port}").
with(body: "{}")
```
This code is split into three chunks by the developer. Each are likely (but not guaranteed) to be intended to stand on their own (in terms of syntax). This behavior is good for scanning neighbors (same indent or higher) within a method, but bad for parsing neighbors across methods.
## Problem
Code is expanded to capture all neighbors, and then it decreases indent level which allows it to capture surrounding scope (think moving from within the method to also capturing the `def/end` definition. Once the indentation level has been increased, we go back to scanning neighbors, but now neighbors also contain keywords.
For example:
```
1 def bark
2
3 end
4
5 def sit
6 end
```
In this case if lines 4, 5, and 6 are in a block when it tries to expand neighbors it will expand up. If it stops after line 2 or 3 it may cause problems since there's a valid kw/end pair, but the block will be checked without it.
TLDR; It's good to stop scanning code after hitting a newline when you're in a method...it causes a problem scanning code between methods when everything inside of one of the methods is an empty line.
In this case it grabs the end on line 3 and since the problem was an extra end, the program now compiles correctly. It incorrectly assumes that the block it captured was causing the problem.
## Extra bit of context
One other technical detail is that after we've decided to stop scanning code for a new neighbor block expansion, we look around the block and grab any empty newlines. Basically adding empty newlines before of after a code block do not affect the parsing of that block.
## The fix
Since we know that this problem only happens when there's a newline inside of a method and we know this particular failure mode is due to having an invalid block (capturing an extra end, but not it's keyword) we have all the metadata we need to detect this scenario and correct it.
We know that the next line above our block must be code or empty (since we grabbed extra newlines). Same for code below it. We can count all the keywords and ends in the block. If they are balanced, it's likely (but not guaranteed) we formed the block correctly. If they're imbalanced, look above or below (depending on the nature of the imbalance), check to see if adding that line would balance the count.
This concept of balance and "leaning" comes from work in https://github.com/ruby/syntax_suggest/pull/152 and has proven useful, but not been formally introduced into the main branch.
## Outcome
Adding this extra check introduced no regressions and fixed the test case. It might be possible there's a mirror or similar problem that we're not handling. That will come out in time. It might also be possible that this causes a worse case in some code not under test. That too would come out in time.
One other possible concern to adding logic in this area (which is a hot codepath), is performance. This extra count check will be performed for every block. In general the two most helpful performance strategies I've found are reducing total number of blocks (therefore reducing overall N internal iterations) and making better matches (the parser to determine if a close block is valid or not is a major bottleneck. If we can split valid code into valid blocks, then it's only evaluated by the parser once, where as invalid code must be continuously re-checked by the parser until it becomes valid, or is determined to be the cause of the core problem.
This extra logic should very rarely result in a change, but when it does it should tend to produce slightly larger blocks (by one line) and more accurate blocks.
Informally it seems to have no impact on performance:
``
This branch:
DEBUG_DISPLAY=1 bundle exec rspec spec/ --format=failures 3.01s user 1.62s system 113% cpu 4.076 total
```
```
On main:
DEBUG_DISPLAY=1 bundle exec rspec spec/ --format=failures 3.02s user 1.64s system 113% cpu 4.098 total
```
https://github.com/ruby/syntax_suggest/commit/13739c6946
When removing comments I previously replaced them with a newline. This loses some context and may affect the order of the indent search which in turn affects the final result. By preserving whitespace in front of the comment, we preserve the "natural" indentation order of the line while also allowing the parser/lexer to see and join naturally consecutive (method chain) lines.
close https://github.com/ruby/syntax_suggest/pull/177
While working on locking multiple platforms by default, I got an
infinite resolution loop in one of our resolver specs.
The culprit ended up being that when dealing with lockfile specs with
incomplete dependencies (spec appears in lockfile, but its dependencies
don't), those specs were not being properly expired and that tripped up
resolution.
The issue for some reason only manifests when dealing with multiple
lockfile platforms, that's why it only manifested when working on
locking multiple platforms by default.
https://github.com/rubygems/rubygems/commit/4ca72913bb
[Feature #18885]
For now, the optimizations performed are:
- Run a major GC
- Compact the heap
- Promote all surviving objects to oldgen
Other optimizations may follow.
When dependencies have changed, we'll be re-resolving, and we can't
really know whether the resolution will be valid or invalid for the Ruby
platform, so skip the removal in that case.
The fix worked, but made some other specs fail, and surfaced that the
`@dependencies_changed` attribute was actually being incorrect set when
explicitly unlocking. Fixed that with an early return.
https://github.com/rubygems/rubygems/commit/20d8f5e5d9
Bundler is very conservative by default, trying to preserve versions
from the lockfile as possible, and never downgrading them. However, when
it runs into a resolution error, it still tries to find a valid
resolution.
This fallback behavior was too "brute-force" though, completely
unrestricting any gem found in the resolution conflict, and that could
lead to direct dependencies being downgraded in some edge cases.
Instead, unlock things a bit more carefully:
* First try unlocking fully pinned indirect dependencies, but leave a
lower bound requirement in place to prevent downgrades.
* Then try unlocking any fully pinned dependency, also leaving a lower
bound requirement in place.
* Finally completely unrestrict dependencies if nothing else worked.
https://github.com/rubygems/rubygems/commit/7f55ed8302
David Rodríguez
Jean Boussier
Nobuyoshi Nakada
Jean Boussier
alexandre184
Hiroshi SHIBATA
Benoit Daloze
Takashi Kokubun
Ellen Marie Dash
Samuel Williams
Gareth Adams
NARUSE, Yui
Peter Zhu
Jean byroot Boussier
schneems
Ivanov-Anton
Martin Dürst
Jeremy Evans
Vladimir Dementyev
git
Aaron Patterson
Josef Šimánek