Skip to content

Commit c7c31bc

Browse files
fix: Fix non-determinism bug due to bad saveSymbolString API.
1 parent dce33e5 commit c7c31bc

File tree

2 files changed

+26
-24
lines changed

2 files changed

+26
-24
lines changed

scip_indexer/SCIPIndexer.cc

Lines changed: 25 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -140,7 +140,6 @@ enum class Emitted {
140140
/// so caches should generally directly or indirectly include a FileRef
141141
/// as part of key (e.g. via core::Loc).
142142
class SCIPState {
143-
string symbolScratchBuffer;
144143
UnorderedMap<UntypedGenericSymbolRef, string> symbolStringCache;
145144

146145
/// Cache of occurrences for locals that have been emitted in this function.
@@ -202,7 +201,7 @@ class SCIPState {
202201

203202
public:
204203
SCIPState(GemMetadata metadata)
205-
: symbolScratchBuffer(), symbolStringCache(), localOccurrenceCache(), symbolOccurrenceCache(),
204+
: symbolStringCache(), localOccurrenceCache(), symbolOccurrenceCache(), potentialRefOnlySymbols(),
206205
gemMetadata(metadata), occurrenceMap(), emittedSymbols(), symbolMap(), documents(), externalSymbols() {}
207206
~SCIPState() = default;
208207
SCIPState(SCIPState &&) = default;
@@ -218,31 +217,35 @@ class SCIPState {
218217
/// If the returned value is as success, the pointer is non-null.
219218
///
220219
/// The argument symbol is used instead of recomputing from scratch if it is non-null.
221-
absl::StatusOr<const string *> saveSymbolString(const core::GlobalState &gs, UntypedGenericSymbolRef symRef,
222-
const scip::Symbol *symbol) {
220+
absl::Status saveSymbolString(const core::GlobalState &gs, UntypedGenericSymbolRef symRef,
221+
const scip::Symbol *symbol, std::string &output) {
223222
auto pair = this->symbolStringCache.find(symRef);
224223
if (pair != this->symbolStringCache.end()) {
225-
return &pair->second;
224+
// Yes, there is a "redundant" string copy here when we could "just"
225+
// optimize it to return an interior pointer into the cache. However,
226+
// that creates a footgun where this method cannot be safely called
227+
// across invocations to non-const method calls on SCIPState.
228+
output = pair->second;
229+
return absl::OkStatus();
226230
}
227231

228-
this->symbolScratchBuffer.clear();
229-
230232
absl::Status status;
231233
if (symbol) {
232-
status = scip::utils::emitSymbolString(*symbol, this->symbolScratchBuffer);
234+
status = scip::utils::emitSymbolString(*symbol, output);
233235
} else {
234236
scip::Symbol symbol;
235237
status = symRef.symbolForExpr(gs, this->gemMetadata, {}, symbol);
236238
if (!status.ok()) {
237239
return status;
238240
}
239-
status = scip::utils::emitSymbolString(symbol, this->symbolScratchBuffer);
241+
status = scip::utils::emitSymbolString(symbol, output);
240242
}
241243
if (!status.ok()) {
242244
return status;
243245
}
244-
symbolStringCache.insert({symRef, this->symbolScratchBuffer});
245-
return &symbolStringCache[symRef];
246+
symbolStringCache.insert({symRef, output});
247+
248+
return absl::OkStatus();
246249
}
247250

248251
private:
@@ -361,9 +364,8 @@ class SCIPState {
361364
SmallVec<scip::Relationship> &rels) {
362365
untypedSymRef.saveRelationships(gs, this->relationshipsMap[file], rels,
363366
[this, &gs](UntypedGenericSymbolRef sym, std::string &out) {
364-
auto status = this->saveSymbolString(gs, sym, nullptr);
367+
auto status = this->saveSymbolString(gs, sym, nullptr, out);
365368
ENFORCE(status.ok());
366-
out = *status.value();
367369
});
368370
}
369371

@@ -397,11 +399,11 @@ class SCIPState {
397399
if (!status.ok()) {
398400
return status;
399401
}
400-
absl::StatusOr<const string *> valueOrStatus(this->saveSymbolString(gs, untypedSymRef, &symbol));
401-
if (!valueOrStatus.ok()) {
402-
return valueOrStatus.status();
402+
std::string symbolString;
403+
status = this->saveSymbolString(gs, untypedSymRef, &symbol, symbolString);
404+
if (!status.ok()) {
405+
return status;
403406
}
404-
const string &symbolString = *valueOrStatus.value();
405407

406408
SmallVec<string> docs;
407409
symRef.saveDocStrings(gs, symRef.definitionType(), occLoc, docs);
@@ -444,11 +446,11 @@ class SCIPState {
444446
}
445447
auto &gs = ctx.state;
446448
auto file = ctx.file;
447-
absl::StatusOr<const string *> valueOrStatus(this->saveSymbolString(gs, symRef.withoutType(), nullptr));
448-
if (!valueOrStatus.ok()) {
449-
return valueOrStatus.status();
449+
std::string symbolString;
450+
auto status = this->saveSymbolString(gs, symRef.withoutType(), nullptr, symbolString);
451+
if (!status.ok()) {
452+
return status;
450453
}
451-
const string &symbolString = *valueOrStatus.value();
452454

453455
SmallVec<string> overrideDocs{};
454456
using Kind = GenericSymbolRef::Kind;
@@ -479,12 +481,11 @@ class SCIPState {
479481
void finalizeRefOnlySymbolInfos(const core::GlobalState &gs, core::FileRef file) {
480482
auto &potentialSyms = this->potentialRefOnlySymbols[file];
481483

484+
std::string symbolString;
482485
for (auto symRef : potentialSyms) {
483-
auto valueOrError = this->saveSymbolString(gs, symRef, nullptr);
484-
if (!valueOrError.ok()) {
486+
if (!this->saveSymbolString(gs, symRef, nullptr, symbolString).ok()) {
485487
continue;
486488
}
487-
auto &symbolString = *valueOrError.value();
488489
// Avoid calling saveRelationships if we already emitted this.
489490
// saveSymbolInfo does this check too, so it isn't strictly needed.
490491
if (this->alreadyEmittedSymbolInfo(file, symbolString)) {

test/scip/testdata/field_inheritance.snapshot.rb

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -29,6 +29,7 @@ def get_inherited_ivar
2929
# ^^ reference [..] C2#`@f`.
3030
# relation definition=[..] C1#`@f`.
3131
# ^^ reference [..] C2#`@h`.
32+
# relation definition=[..] C1#`@h`.
3233
end
3334

3435
def set_inherited_ivar

0 commit comments

Comments
 (0)