3663 - fix a refcounting bug: '(type)' != 'type'

This was a large commit, and most of it is a follow-up to commit 3309,
undoing what is probably the final ill-considered optimization I added
to s-expressions in Mu: I was always representing (a b c) as (a b . c),
etc. That is now gone.

Why did I need to take it out? The key problem was the error silently
ignored in layer 30. That was causing size_of("(type)") to silently
return garbage rather than loudly complain (assuming 'type' was a simple
type).

But to take it out I had to modify types_strictly_match (layer 21) to
actually strictly match and not just do a prefix match.

In the process of removing the prefix match, I had to make extracting
recipe types from recipe headers more robust. So far it only matched the
first element of each ingredient's type; these matched:

  (recipe address:number -> address:number)
  (recipe address -> address)

I didn't notice because the dotted notation optimization was actually
representing this as:

  (recipe address:number -> address number)

---

One final little thing in this commit: I added an alias for 'assert'
called 'assert_for_now', to indicate that I'm not sure something's
really an invariant, that it might be triggered by (invalid) user
programs, and so require more thought on error handling down the road.

But this may well be an ill-posed distinction. It may be overwhelmingly
uneconomic to continually distinguish between model invariants and error
states for input. I'm starting to grow sympathetic to Google Analytics's
recent approach of just banning assertions altogether. We'll see..
This commit is contained in:
Kartik K. Agaram 2016-11-10 21:39:02 -08:00
parent dc80c7b92a
commit 93d4cc937e
16 changed files with 201 additions and 65 deletions

View File

@ -176,6 +176,9 @@ int Trace_errors = 0; // used only when Trace_stream is NULL
// Errors are a special layer.
#define raise (!Trace_stream ? (tb_shutdown(),++Trace_errors,cerr) /*do print*/ : Trace_stream->stream(Error_depth, "error"))
// If we aren't yet sure how to deal with some corner case, use assert_for_now
// to indicate that it isn't an inviolable invariant.
#define assert_for_now assert
// Inside tests, fail any tests that displayed (unexpected) errors.
// Expected errors in tests should always be hidden and silently checked for.

View File

@ -301,12 +301,34 @@ void slurp_properties(istream& in, vector<pair<string, string_tree*> >& out) {
string_tree* parse_property_list(istream& in) {
skip_whitespace_but_not_newline(in);
if (!has_data(in)) return NULL;
string_tree* left = new string_tree(slurp_until(in, ':'));
if (!has_data(in)) return left;
string_tree* right = parse_property_list(in);
return new string_tree(left, right);
string_tree* first = new string_tree(slurp_until(in, ':'));
if (!has_data(in)) return first;
string_tree* rest = parse_property_list(in);
if (!has_data(in) && rest->atom)
return new string_tree(first, new string_tree(rest, NULL));
return new string_tree(first, rest);
}
:(before "End Unit Tests")
void test_parse_property_list_atom() {
istringstream in("a");
string_tree* x = parse_property_list(in);
CHECK(x->atom);
delete x;
}
void test_parse_property_list_list() {
istringstream in("a:b");
string_tree* x = parse_property_list(in);
CHECK(!x->atom);
CHECK(x->left->atom);
CHECK_EQ(x->left->value, "a");
CHECK(!x->right->atom);
CHECK(x->right->left->atom);
CHECK_EQ(x->right->left->value, "b");
CHECK(x->right->right == NULL);
delete x;
}
:(code)
type_tree* new_type_tree(const string_tree* properties) {
if (!properties) return NULL;
if (properties->atom) {
@ -375,9 +397,10 @@ bool type_tree::operator<(const type_tree& other) const {
if (!left && other.left) return true;
if (right && !other.right) return false;
if (!right && other.right) return true;
// now if either pointer is unequal neither side can be null
// if one side is equal that's easy
if (left == other.left || *left == *other.left) return *right < *other.right;
if (right == other.right || *right == *other.right) return *left < *other.left;
if (left == other.left || *left == *other.left) return right && *right < *other.right;
if (right == other.right || *right == *other.right) return left && *left < *other.left;
// if the two sides criss-cross, pick the side with the smaller lhs
if ((left == other.right || *left == *other.right)
&& (right == other.left || *right == *other.left))
@ -423,15 +446,11 @@ void test_compare_list_with_extra_element() {
}
void test_compare_list_with_smaller_left_but_larger_right() {
reagent a("a:address:number"), b("b:character:array");
assert(a.type->left->atom && a.type->right->atom);
assert(b.type->left->atom && b.type->right->atom);
CHECK(*a.type < *b.type);
CHECK(!(*b.type < *a.type));
}
void test_compare_list_with_smaller_left_but_larger_right_identical_types() {
reagent a("a:address:boolean"), b("b:boolean:address");
assert(a.type->left->atom && a.type->right->atom);
assert(b.type->left->atom && b.type->right->atom);
CHECK(*a.type < *b.type);
CHECK(!(*b.type < *a.type));
}
@ -658,8 +677,11 @@ void dump(const string_tree* x, ostream& out) {
if (curr->right) out << ' ';
curr = curr->right;
}
// final right
dump(curr, out);
// check for dotted list; should never happen
if (curr) {
out << ". ";
dump(curr, out);
}
out << ')';
}
@ -684,8 +706,11 @@ void dump(const type_tree* x, ostream& out) {
if (curr->right) out << ' ';
curr = curr->right;
}
// final right
dump(curr, out);
// check for dotted list; should never happen
if (curr) {
out << ". ";
dump(curr, out);
}
out << ')';
}
@ -717,8 +742,11 @@ void dump_names(const type_tree* x, ostream& out) {
if (curr->right) out << ' ';
curr = curr->right;
}
// final right
dump_names(curr, out);
// check for dotted list; should never happen
if (curr) {
out << ". ";
dump_names(curr, out);
}
out << ')';
}
@ -743,8 +771,11 @@ void dump_names_without_quotes(const type_tree* x, ostream& out) {
if (curr->right) out << ' ';
curr = curr->right;
}
// final right
dump_names_without_quotes(curr, out);
// check for dotted list; should never happen
if (curr) {
out << ". ";
dump_names_without_quotes(curr, out);
}
out << ')';
}

View File

@ -4,9 +4,8 @@
// (address to array of charaters) to (list of numbers)".
//
// Type trees aren't as general as s-expressions even if they look like them:
// the first element of a type tree is always an atom, and left and right
// pointers of non-atoms are never NULL. All type trees are 'dotted' in lisp
// parlance.
// the first element of a type tree is always an atom, and it can never be
// dotted (right->right->right->...->right is always NULL).
//
// For now you can't use the simpler 'colon-based' representation inside type
// trees. Once you start typing parens, keep on typing parens.
@ -79,19 +78,6 @@ string_tree* parse_string_tree(istream& in) {
}
in.get(); // skip ')'
assert(*curr == NULL);
if (result == NULL) return result;
if (result->right == NULL) return result;
// standardize the final element to always be on the right if it's an atom
// (a b c) => (a b . c) in s-expression parlance
string_tree* tmp = result;
while (tmp->right->right) tmp = tmp->right;
assert(!tmp->right->atom);
if (!tmp->right->left->atom) return result;
string_tree* tmp2 = tmp->right;
tmp->right = tmp2->left;
tmp2->left = NULL;
assert(tmp2->right == NULL);
delete tmp2;
return result;
}

View File

@ -102,6 +102,7 @@ bool types_coercible(const reagent& to, const reagent& from) {
if (is_mu_address(from) && is_mu_number(to)) return true;
if (is_mu_boolean(from) && is_mu_number(to)) return true;
if (is_mu_number(from) && is_mu_boolean(to)) return true;
// End types_coercible Special-cases
return false;
}
@ -136,14 +137,12 @@ bool types_strictly_match(reagent/*copy*/ to, reagent/*copy*/ from) {
return types_strictly_match(to.type, from.type);
}
// two types match if the second begins like the first
// (trees perform the same check recursively on each subtree)
bool types_strictly_match(const type_tree* to, const type_tree* from) {
if (from == to) return true;
if (!to) return false;
if (!from) return to->atom && to->value == 0;
if (to->atom && !from->atom) return from->left->atom && from->left->name == to->name;
if (from->atom != to->atom) return false;
if (from->atom) {
if (!to->atom) return false;
if (from->value == -1) return from->name == to->name;
return from->value == to->value;
}

View File

@ -182,6 +182,7 @@ bool is_mu_text(reagent/*copy*/ x) {
&& x.type->right->left->atom
&& x.type->right->left->value == get(Type_ordinal, "array")
&& x.type->right->right
&& x.type->right->right->atom
&& x.type->right->right->value == get(Type_ordinal, "character");
&& !x.type->right->right->atom
&& x.type->right->right->left->value == get(Type_ordinal, "character")
&& x.type->right->right->right == NULL;
}

View File

@ -157,7 +157,11 @@ if (!contains_key(Type, base_type->value)) {
type_info t = get(Type, base_type->value);
if (t.kind == CONTAINER) {
// Compute size_of Container
if (!contains_key(Container_metadata, type)) return 1; // error raised elsewhere
if (!contains_key(Container_metadata, type)) {
raise << "unknown size for container type '" << to_string(type) << "'\n" << end();
//? DUMP("");
return 0;
}
return get(Container_metadata, type).size;
}
@ -203,7 +207,7 @@ void compute_container_sizes(const type_tree* type, set<type_tree>& pending_meta
return;
}
if (type->left->name == "address")
compute_container_sizes(type->right, pending_metadata, location_for_error_messages);
compute_container_sizes(payload_type(type), pending_metadata, location_for_error_messages);
// End compute_container_sizes Non-atom Special-cases
return;
}
@ -231,6 +235,14 @@ void compute_container_sizes(const type_info& container_info, const type_tree* f
Container_metadata.push_back(pair<type_tree*, container_metadata>(new type_tree(*full_type), metadata));
}
const type_tree* payload_type(const type_tree* type) {
assert(!type->atom);
const type_tree* result = type->right;
assert(!result->atom);
if (!result->right) return result->left;
return result;
}
container_metadata& get(vector<pair<type_tree*, container_metadata> >& all, const type_tree* key) {
for (int i = 0; i < SIZE(all); ++i) {
if (matches(all.at(i).first, key))

View File

@ -98,6 +98,10 @@ def main [
]
+app: foo: 3 14 15 16
:(before "End types_coercible Special-cases")
if (is_mu_array(from) && is_mu_array(to))
return types_strictly_match(array_element(from.type), array_element(to.type));
:(before "End size_of(reagent r) Special-cases")
if (!r.type->atom && r.type->left->atom && r.type->left->value == get(Type_ordinal, "array")) {
if (!r.type->right) {
@ -111,10 +115,10 @@ if (!r.type->atom && r.type->left->atom && r.type->left->value == get(Type_ordin
if (type->left->value == get(Type_ordinal, "array")) return static_array_length(type);
:(code)
int static_array_length(const type_tree* type) {
if (!type->atom && !type->right->atom && type->right->right->atom // exactly 3 types
&& is_integer(type->right->right->name)) { // third 'type' is a number
if (!type->atom && type->right && !type->right->atom && type->right->right && !type->right->right->atom && !type->right->right->right // exactly 3 types
&& type->right->right->left && type->right->right->left->atom && is_integer(type->right->right->left->name)) { // third 'type' is a number
// get size from type
return to_integer(type->right->right->name);
return to_integer(type->right->right->left->name);
}
cerr << to_string(type) << '\n';
assert(false);
@ -159,7 +163,7 @@ container foo [
raise << "container '" << name << "' doesn't specify type of array elements for '" << info.elements.back().name << "'\n" << end();
continue;
}
if (type->right->atom) { // array has no length
if (!type->right->right || !is_integer(type->right->right->left->name)) { // array has no length
raise << "container '" << name << "' cannot determine size of element '" << info.elements.back().name << "'\n" << end();
continue;
}
@ -365,20 +369,29 @@ type_tree* copy_array_element(const type_tree* type) {
type_tree* array_element(const type_tree* type) {
assert(type->right);
// hack: don't require parens for either array:num:3 array:address:num
if (!type->right->atom && type->right->right && type->right->right->atom && is_integer(type->right->right->name))
if (type->right->atom) {
return type->right;
}
else if (!type->right->right) {
return type->right->left;
}
// hack: support array:num:3 without requiring extra parens
else if (type->right->right->left && type->right->right->left->atom && is_integer(type->right->right->left->name)) {
assert(!type->right->right->right);
return type->right->left;
}
return type->right;
}
int array_length(const reagent& x) {
// x should already be canonized.
if (!x.type->atom && !x.type->right->atom && x.type->right->right->atom // exactly 3 types
&& is_integer(x.type->right->right->name)) { // third 'type' is a number
// hack: look for length in type
if (!x.type->atom && x.type->right && !x.type->right->atom && x.type->right->right && !x.type->right->right->atom && !x.type->right->right->right // exactly 3 types
&& x.type->right->right->left && x.type->right->right->left->atom && is_integer(x.type->right->right->left->name)) { // third 'type' is a number
// get size from type
return to_integer(x.type->right->right->name);
return to_integer(x.type->right->right->left->name);
}
// we should never get here at transform time
// this should never happen at transform time
return get_or_insert(Memory, x.value);
}
@ -393,6 +406,11 @@ void test_array_length_compound() {
CHECK_EQ(array_length(x), 3);
}
void test_array_length_static() {
reagent x("1:array:num:3");
CHECK_EQ(array_length(x), 3);
}
:(scenario index_truncates)
def main [
1:array:num:3 <- create-array

View File

@ -210,10 +210,18 @@ void drop_from_type(reagent& r, string expected_type) {
raise << "can't drop2 " << expected_type << " from '" << to_string(r) << "'\n" << end();
return;
}
// r.type = r.type->right
type_tree* tmp = r.type;
r.type = tmp->right;
tmp->right = NULL;
delete tmp;
// if (!r.type->right) r.type = r.type->left
assert(!r.type->atom);
if (r.type->right) return;
tmp = r.type;
r.type = tmp->left;
tmp->left = NULL;
delete tmp;
}
:(scenario new_returns_incorrect_type)
@ -223,6 +231,13 @@ def main [
]
+error: main: product of 'new' has incorrect type: '1:bool <- new num:type'
:(scenario new_discerns_singleton_list_from_atom_container)
% Hide_errors = true;
def main [
1:address:num/raw <- new {(num): type} # should be '{num: type}'
]
+error: main: product of 'new' has incorrect type: '1:address:num/raw <- new {(num): type}'
:(scenario new_with_type_abbreviation)
def main [
1:address:num/raw <- new num:type

View File

@ -52,7 +52,7 @@ void decrement_any_refcounts(const reagent& canonized_x) {
if (is_mu_address(canonized_x)) {
assert(canonized_x.value);
assert(!canonized_x.metadata.size);
decrement_refcount(get_or_insert(Memory, canonized_x.value), canonized_x.type->right, payload_size(canonized_x));
decrement_refcount(get_or_insert(Memory, canonized_x.value), payload_type(canonized_x.type), payload_size(canonized_x));
}
// End Decrement Refcounts(canonized_x)
}
@ -316,7 +316,7 @@ void compute_container_address_offsets(const type_tree* type, const string& loca
return;
}
if (type->left->name == "address")
compute_container_address_offsets(type->right, location_for_error_messages);
compute_container_address_offsets(payload_type(type), location_for_error_messages);
else if (type->left->name == "array")
compute_container_address_offsets(array_element(type), location_for_error_messages);
// End compute_container_address_offsets Non-atom Special-cases
@ -352,7 +352,7 @@ void compute_exclusive_container_address_offsets(const type_info& exclusive_cont
void append_addresses(int base_offset, const type_tree* type, map<set<tag_condition_info>, set<address_element_info> >& out, const set<tag_condition_info>& key, const string& location_for_error_messages) {
if (is_mu_address(type)) {
get_or_insert(out, key).insert(address_element_info(base_offset, new type_tree(*type->right)));
get_or_insert(out, key).insert(address_element_info(base_offset, new type_tree(*payload_type(type))));
return;
}
const type_tree* base_type = type;
@ -365,7 +365,7 @@ void append_addresses(int base_offset, const type_tree* type, map<set<tag_condit
// Compute Container Address Offset(element)
if (is_mu_address(element)) {
trace(9993, "transform") << "address at offset " << curr_offset << end();
get_or_insert(out, key).insert(address_element_info(curr_offset, new type_tree(*element.type->right)));
get_or_insert(out, key).insert(address_element_info(curr_offset, new type_tree(*payload_type(element.type))));
++curr_offset;
}
else if (is_mu_array(element)) {

View File

@ -121,9 +121,13 @@ type_ordinal skip_addresses(type_tree* type) {
while (type && is_compound_type_starting_with(type, "address"))
type = type->right;
if (!type) return -1; // error handled elsewhere
if (type->atom) return type->value;
const type_tree* base_type = type;
// Update base_type in skip_addresses
return base_type->value;
if (base_type->atom)
return base_type->value;
assert(base_type->left->atom);
return base_type->left->value;
}
int find_element_name(const type_ordinal t, const string& name, const string& recipe_name) {

View File

@ -198,8 +198,8 @@ if (x.name == "number-of-locals") {
:(scenario local_scope)
def main [
1:& <- foo
2:& <- foo
1:&:@:location <- foo
2:&:@:location <- foo
3:bool <- equal 1:&, 2:&
]
def foo [

View File

@ -392,7 +392,7 @@ void check_memory(const string& s) {
void check_type(const string& lhs, istream& in) {
reagent x(lhs);
if (is_mu_array(x.type) && is_mu_character(x.type->right)) {
if (is_mu_array(x.type) && is_mu_character(array_element(x.type))) {
x.set_value(to_integer(x.name));
skip_whitespace_and_comments(in);
string _assign = next_word(in);

View File

@ -353,12 +353,41 @@ void replace_type_ingredients(type_tree* element_type, const type_tree* callsite
if (!callsite_type) return; // error but it's already been raised above
if (!element_type) return;
if (!element_type->atom) {
if (element_type->right == NULL && is_type_ingredient(element_type->left)) {
int type_ingredient_index = to_type_ingredient_index(element_type->left);
if (corresponding(callsite_type, type_ingredient_index, is_final_type_ingredient(type_ingredient_index, container_info))->right) {
// replacing type ingredient at end of list, and replacement is a non-degenerate compound type -- (a b) but not (a)
replace_type_ingredient_at(type_ingredient_index, element_type, callsite_type, container_info, location_for_error_messages);
return;
}
}
replace_type_ingredients(element_type->left, callsite_type, container_info, location_for_error_messages);
replace_type_ingredients(element_type->right, callsite_type, container_info, location_for_error_messages);
return;
}
if (element_type->value < START_TYPE_INGREDIENTS) return;
const int type_ingredient_index = element_type->value-START_TYPE_INGREDIENTS;
if (is_type_ingredient(element_type))
replace_type_ingredient_at(to_type_ingredient_index(element_type), element_type, callsite_type, container_info, location_for_error_messages);
}
const type_tree* corresponding(const type_tree* type, int index, bool final) {
for (const type_tree* curr = type; curr; curr = curr->right, --index) {
assert_for_now(!curr->atom);
if (index == 0)
return final ? curr : curr->left;
}
assert_for_now(false);
}
bool is_type_ingredient(const type_tree* type) {
return type->atom && type->value >= START_TYPE_INGREDIENTS;
}
int to_type_ingredient_index(const type_tree* type) {
assert(type->atom);
return type->value-START_TYPE_INGREDIENTS;
}
void replace_type_ingredient_at(const int type_ingredient_index, type_tree* element_type, const type_tree* callsite_type, const type_info& container_info, const string& location_for_error_messages) {
if (!has_nth_type(callsite_type, type_ingredient_index)) {
raise << "illegal type " << names_to_string(callsite_type) << " seems to be missing a type ingredient or three" << location_for_error_messages << '\n' << end();
return;
@ -450,6 +479,15 @@ void test_replace_last_type_ingredient_with_multiple() {
CHECK_EQ(to_string(element2), "{y: (\"address\" \"array\" \"character\")}");
}
void test_replace_last_type_ingredient_inside_compound() {
run("container foo:_a:_b [\n"
" {x: (bar _a (address _b))}\n"
"]\n");
reagent callsite("f:foo:number:array:character");
reagent element = element_type(callsite.type, 0);
CHECK_EQ(names_to_string_without_quotes(element.type), "(bar number (address array character))");
}
void test_replace_middle_type_ingredient_with_multiple() {
run("container foo:_a:_b:_c [\n"
" x:_a\n"

View File

@ -177,6 +177,7 @@ bool concrete_type_names_strictly_match(const type_tree* to, const type_tree* fr
if (!to) return !from;
if (!from) return !to;
if (to->atom && is_type_ingredient_name(to->name)) return true; // type ingredient matches anything
if (!to->atom && to->right == NULL && to->left != NULL && to->left->atom && is_type_ingredient_name(to->left->name)) return true;
if (from->atom && is_mu_address(to))
return from->name == "literal" && rhs_reagent.name == "0";
if (!from->atom && !to->atom)
@ -328,6 +329,10 @@ void accumulate_type_ingredients(const type_tree* exemplar_type, const type_tree
raise << " (called from '" << to_original_string(call_instruction) << "')\n" << end();
return;
}
if (!exemplar_type->atom && exemplar_type->right == NULL && !refinement_type->atom && refinement_type->right != NULL) {
exemplar_type = exemplar_type->left;
assert_for_now(exemplar_type->atom);
}
if (exemplar_type->atom) {
if (is_type_ingredient_name(exemplar_type->name)) {
const type_tree* curr_refinement_type = NULL; // temporary heap allocation; must always be deleted before it goes out of scope
@ -402,6 +407,10 @@ void replace_type_ingredients(reagent& x, const map<string, const type_tree*>& m
void replace_type_ingredients(type_tree* type, const map<string, const type_tree*>& mappings) {
if (!type) return;
if (!type->atom) {
if (type->right == NULL && type->left != NULL && type->left->atom && contains_key(mappings, type->left->name) && !get(mappings, type->left->name)->atom && get(mappings, type->left->name)->right != NULL) {
*type = *get(mappings, type->left->name);
return;
}
replace_type_ingredients(type->left, mappings);
replace_type_ingredients(type->right, mappings);
return;
@ -656,6 +665,7 @@ void test_shape_shifting_new_ingredient_does_not_pollute_global_namespace() {
CHECK(!element.type->right);
}
//: specializing a type ingredient with a compound type
:(scenario shape_shifting_recipe_supports_compound_types)
def main [
1:&:point <- new point:type
@ -670,6 +680,22 @@ def bar a:_t -> result:_t [
]
+mem: storing 34 in location 5
//: specializing a type ingredient with a compound type -- while *inside* another compound type
:(scenario shape_shifting_recipe_supports_compound_types_2)
container foo:_t [
value:_t
]
def bar x:&:foo:_t -> result:_t [
local-scope
load-ingredients
result <- get *x, value:offset
]
def main [
1:&:foo:&:point <- new {(foo address point): type}
2:&:point <- bar 1:&:foo:&:point
]
# no errors; call to 'bar' successfully specialized
:(scenario shape_shifting_recipe_error)
% Hide_errors = true;
def main [

View File

@ -262,7 +262,7 @@ reagent next_recipe_reagent(const type_tree* curr) {
if (!curr->left) return reagent("recipe:"+curr->name);
reagent result;
result.name = "recipe";
result.type = new type_tree(*curr->left);
result.type = new type_tree(*curr);
return result;
}

View File

@ -218,7 +218,7 @@ case DEEP_COPY: {
products.push_back(deep_copy(input, tmp));
// reclaim Mu memory allocated for tmp
trace(9991, "run") << "deep-copy: reclaiming temporary" << end();
abandon(tmp.value, tmp.type->right, payload_size(tmp));
abandon(tmp.value, payload_type(tmp.type), payload_size(tmp));
// reclaim host memory allocated for tmp.type when tmp goes out of scope
break;
}
@ -283,7 +283,10 @@ void deep_copy(const reagent& canonized_in, map<int, int>& addresses_copied, con
// construct a fake reagent that reads directly from the appropriate
// field of the container
reagent curr;
curr.type = new type_tree(new type_tree("address"), new type_tree(*info->payload_type));
if (info->payload_type->atom)
curr.type = new type_tree(new type_tree("address"), new type_tree(new type_tree(info->payload_type->name), NULL));
else
curr.type = new type_tree(new type_tree("address"), new type_tree(*info->payload_type));
curr.set_value(canonized_in.value + info->offset);
curr.properties.push_back(pair<string, string_tree*>("raw", NULL));
trace(9991, "run") << "deep-copy: copying address " << curr.value << end();