Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Detect overlapping field names #147

Merged
merged 2 commits into from
Jun 10, 2020
Merged

Detect overlapping field names #147

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5e00d91
Error when a user tries to redefine a field used in a superclass.
ltratt Jun 9, 2020
4d6ba76
Don't allow the user to define the same field name in a class more th…
ltratt Jun 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 13 additions & 0 deletions lang_tests/instance_fields_overlap/test.som
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
"
VM:
status: error
stderr:
...test.som', line 10, column 23:
test = test_super ( | a |
Field 'a' is already defined in a superclass.
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure if this should be "Field" or (as below) "Field name". Any thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

I as non-native speak would say: 'Field a is already defined' or 'A field with the name a is already defined'.
"Field name 'a' has already been defined in this class." feels wrong.
Perhaps 'A field named 'a' ...' as another option.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, I think on reflection that "Field" is better than "Field name" (due to the fields method being a precedent).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in f57c74b.

Copy link
Member

Choose a reason for hiding this comment

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

Or even better "A field by the name 'a' is already defined...".

Copy link
Contributor

Choose a reason for hiding this comment

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

The most useful improvement would IMHO be to name the superclass and the source location where the conflicting field is defined. That would be very developer-friendly :)

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think error messages need to use full grammar (so to speak): it can often end up being pointless extra verbiage to plough through especially when you tend to see the same messages over and over again. I think I'd prefer to keep it as in f57c74b.

"

test = test_super ( | a |
run = (
)
)
3 changes: 3 additions & 0 deletions lang_tests/instance_fields_overlap/test_super.som
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
test_super = (
| a |
)
12 changes: 12 additions & 0 deletions lang_tests/instance_fields_overlap2.som
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
"
VM:
status: error
stderr:
...instance_fields_overlap2.som', line 11, column 9:
| a a |
Field 'a' has already been defined in this class.
"

instance_field_overlap2 = (
| a a |
)
25 changes: 24 additions & 1 deletion src/lib/compiler/ast_to_instrs.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -153,13 +153,36 @@ impl<'a, 'input> Compiler<'a, 'input> {
.iter()
.map(|(k, v)| (k.to_owned(), *v)),
);
for var in ast_inst_vars {
let n = lexer.span_str(*var);
if inst_vars.contains_key(n) {
return Err(vec![(
*var,
format!("Field '{}' is already defined in a superclass.", n),
)]);
}
}
inst_vars
} else {
HashMap::with_capacity(ast_inst_vars.len())
};
for var in ast_inst_vars {
let vars_len = inst_vars.len();
inst_vars.insert(lexer.span_str(*var).to_owned(), vars_len);
let n = lexer.span_str(*var).to_owned();
match inst_vars.entry(n) {
hash_map::Entry::Occupied(_) => {
return Err(vec![(
*var,
format!(
"Field '{}' has already been defined in this class.",
self.lexer.span_str(*var)
),
)])
}
hash_map::Entry::Vacant(e) => {
e.insert(vars_len);
}
}
}
self.vars_stack.push(inst_vars);

Expand Down