Skip to content

Commit

Permalink
Fix issue rust-lang#3212: Allow empty structure
Browse files Browse the repository at this point in the history
The fix is straight-forward, but there are several changes
while fixing the issue.

    1) disallow `mut` keyword when making a new struct

      In code base, there are following code,

      struct Foo { mut a: int };
      let a = Foo { mut a: 1 };

      This is because of structural record, which is
      deprecated corrently (see issue rust-lang#3089) In structural
      record, `mut` keyword should be allowd to control
      mutability. But without structural record, we don't
      need to allow `mut` keyword while constructing struct.

    2) disallow structural records in parser level
      This is related to 1). With structural records, there
      is an ambiguity between empty block and empty struct
      To solve the problem, I change parser to stop parsing
      structural records. I think this is not a problem,
      because structural records are not compiled already.

Misc. issues

There is an ambiguity between empty struct vs. empty match stmt.
with following code,

match x{} {}

Two interpretation is possible, which is listed blow

match (x{}) {} //  matching with newly-constructed empty struct
(match x{}) {}  //  matching with empty enum(or struct) x
                //  and then empty block

It seems that there is no such code in rust code base, but
there is one test which uses empty match statement:
https://github.com/mozilla/rust/blob/incoming/src/test/run-pass/issue-3037.rs

All other cases could be distinguished with look-ahead,
but this can't be. One possible solution is wrapping with
parentheses when matching with an uninhabited type.

enum what { }
fn match_with_empty(x: what) -> ~str {
    match (x) { //use parentheses to remove the ambiguity
    }
}
  • Loading branch information
yjh0502 committed Feb 27, 2013
1 parent 93a7f23 commit 759702f
Show file tree
Hide file tree
Showing 11 changed files with 96 additions and 65 deletions.
6 changes: 3 additions & 3 deletions src/libcore/dvec.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,17 +62,17 @@ pub struct DVec<A> {

/// Creates a new, empty dvec
pub pure fn DVec<A>() -> DVec<A> {
DVec {mut data: ~[]}
DVec {data: ~[]}
}

/// Creates a new dvec with a single element
pub pure fn from_elem<A>(e: A) -> DVec<A> {
DVec {mut data: ~[e]}
DVec {data: ~[e]}
}

/// Creates a new dvec with the contents of a vector
pub pure fn from_vec<A>(v: ~[A]) -> DVec<A> {
DVec {mut data: v}
DVec {data: v}
}

/// Consumes the vector and returns its contents
Expand Down
16 changes: 10 additions & 6 deletions src/libcore/os.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,8 +322,8 @@ pub struct Pipe { mut in: c_int, mut out: c_int }
#[cfg(unix)]
pub fn pipe() -> Pipe {
unsafe {
let mut fds = Pipe {mut in: 0 as c_int,
mut out: 0 as c_int };
let mut fds = Pipe {in: 0 as c_int,
out: 0 as c_int };
assert (libc::pipe(&mut fds.in) == (0 as c_int));
return Pipe {in: fds.in, out: fds.out};
}
Expand All @@ -339,8 +339,8 @@ pub fn pipe() -> Pipe {
// fully understand. Here we explicitly make the pipe non-inheritable,
// which means to pass it to a subprocess they need to be duplicated
// first, as in rust_run_program.
let mut fds = Pipe { mut in: 0 as c_int,
mut out: 0 as c_int };
let mut fds = Pipe {in: 0 as c_int,
out: 0 as c_int };
let res = libc::pipe(&mut fds.in, 1024 as c_uint,
(libc::O_BINARY | libc::O_NOINHERIT) as c_int);
assert (res == 0 as c_int);
Expand Down Expand Up @@ -566,13 +566,17 @@ pub fn path_exists(p: &Path) -> bool {
*
* If the given path is relative, return it prepended with the current working
* directory. If the given path is already an absolute path, return it
* as is. This is a shortcut for calling os::getcwd().unsafe_join(p)
* as is.
*/
// NB: this is here rather than in path because it is a form of environment
// querying; what it does depends on the process working directory, not just
// the input paths.
pub fn make_absolute(p: &Path) -> Path {
getcwd().unsafe_join(p)
if p.is_absolute {
copy *p
} else {
getcwd().push_many(p.components)
}
}


Expand Down
4 changes: 2 additions & 2 deletions src/libcore/ptr.rs
Original file line number Diff line number Diff line change
Expand Up @@ -300,15 +300,15 @@ impl<T:Ord> Ord for &const T {
pub fn test() {
unsafe {
struct Pair {mut fst: int, mut snd: int};
let mut p = Pair {mut fst: 10, mut snd: 20};
let mut p = Pair {fst: 10, snd: 20};
let pptr: *mut Pair = &mut p;
let iptr: *mut int = cast::reinterpret_cast(&pptr);
assert (*iptr == 10);;
*iptr = 30;
assert (*iptr == 30);
assert (p.fst == 30);;

*pptr = Pair {mut fst: 50, mut snd: 60};
*pptr = Pair {fst: 50, snd: 60};
assert (*iptr == 50);
assert (p.fst == 50);
assert (p.snd == 60);
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/ebml.rs
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ pub mod reader {
}

pub fn Decoder(d: Doc) -> Decoder {
Decoder { mut parent: d, mut pos: d.start }
Decoder { parent: d, pos: d.start }
}

priv impl Decoder {
Expand Down
2 changes: 1 addition & 1 deletion src/libstd/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ enum Sem<Q> = Exclusive<SemInner<Q>>;
#[doc(hidden)]
fn new_sem<Q:Owned>(count: int, q: Q) -> Sem<Q> {
Sem(exclusive(SemInner {
mut count: count, waiters: new_waitqueue(), blocked: q }))
count: count, waiters: new_waitqueue(), blocked: q }))
}
#[doc(hidden)]
fn new_sem_and_signal(count: int, num_condvars: uint)
Expand Down
106 changes: 57 additions & 49 deletions src/libsyntax/parse/parser.rs
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ use parse::obsolete::{ObsoleteMoveInit, ObsoleteBinaryMove};
use parse::obsolete::{ObsoleteStructCtor, ObsoleteWith};
use parse::obsolete::{ObsoleteSyntax, ObsoleteLowerCaseKindBounds};
use parse::obsolete::{ObsoleteUnsafeBlock, ObsoleteImplSyntax};
use parse::obsolete::{ObsoleteTraitBoundSeparator, ObsoleteMutOwnedPointer};
use parse::obsolete::{ObsoleteTraitBoundSeparator};
use parse::prec::{as_prec, token_to_binop};
use parse::token::{can_begin_expr, is_ident, is_ident_or_path};
use parse::token::{is_plain_ident, INTERPOLATED, special_idents};
Expand Down Expand Up @@ -677,11 +677,6 @@ pub impl Parser {
// rather than boxed ptrs. But the special casing of str/vec is not
// reflected in the AST type.
let mt = self.parse_mt();

if mt.mutbl == m_mutbl && sigil == OwnedSigil {
self.obsolete(*self.last_span, ObsoleteMutOwnedPointer);
}

ctor(mt)
}

Expand Down Expand Up @@ -753,6 +748,18 @@ pub impl Parser {
}
}

fn parse_capture_item_or(parse_arg_fn: fn(Parser) -> arg_or_capture_item)
-> arg_or_capture_item
{
if self.eat_keyword(~"copy") {
// XXX outdated syntax now that moves-based-on-type has gone in
self.parse_ident();
either::Right(())
} else {
parse_arg_fn(self)
}
}

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Feb 27, 2013

this looks like a merge failure. the code to parse capture clauses was removed I believe? not sure why you'd be adding it back in as part of this patch.

This comment has been minimized.

Copy link
@yjh0502

yjh0502 Feb 27, 2013

Author Owner

That's my mistake. I'll add another patch to fix the problem.

// This version of parse arg doesn't necessarily require
// identifier names.
fn parse_arg_general(require_name: bool) -> arg {
Expand Down Expand Up @@ -781,26 +788,32 @@ pub impl Parser {
either::Left(self.parse_arg_general(true))
}
fn parse_arg_or_capture_item() -> arg_or_capture_item {
self.parse_capture_item_or(|p| p.parse_arg())
}

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Feb 27, 2013

likewise (merge failure?)

fn parse_fn_block_arg() -> arg_or_capture_item {
let m = self.parse_arg_mode();
let is_mutbl = self.eat_keyword(~"mut");
let pat = self.parse_pat(false);
let t = if self.eat(token::COLON) {
self.parse_ty(false)
} else {
@Ty {
id: self.get_id(),
node: ty_infer,
span: mk_sp(self.span.lo, self.span.hi),
}
};
either::Left(ast::arg {
mode: m,
is_mutbl: is_mutbl,
ty: t,
pat: pat,
id: self.get_id()
})

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Feb 27, 2013

likewise (merge failure?)

do self.parse_capture_item_or |p| {
let m = p.parse_arg_mode();
let is_mutbl = self.eat_keyword(~"mut");
let pat = p.parse_pat(false);
let t = if p.eat(token::COLON) {
p.parse_ty(false)
} else {
@Ty {
id: p.get_id(),
node: ty_infer,
span: mk_sp(p.span.lo, p.span.hi),
}
};
either::Left(ast::arg {
mode: m,
is_mutbl: is_mutbl,
ty: t,
pat: pat,
id: p.get_id()
})
}
}

fn maybe_parse_fixed_vstore_with_star() -> Option<uint> {
Expand Down Expand Up @@ -1095,15 +1108,10 @@ pub impl Parser {
self.mk_expr(lo, hi, expr_tup(es))
}
} else if *self.token == token::LBRACE {
if self.looking_at_record_literal() {
ex = self.parse_record_literal();
hi = self.span.hi;
} else {
self.bump();
let blk = self.parse_block_tail(lo, default_blk);
return self.mk_expr(blk.span.lo, blk.span.hi,
expr_block(blk));
}
self.bump();
let blk = self.parse_block_tail(lo, default_blk);
return self.mk_expr(blk.span.lo, blk.span.hi,
expr_block(blk));
} else if token::is_bar(*self.token) {
return self.parse_lambda_expr();
} else if self.eat_keyword(~"if") {
Expand Down Expand Up @@ -1221,24 +1229,21 @@ pub impl Parser {
self.bump();
let mut fields = ~[];
let mut base = None;
fields.push(self.parse_field(token::COLON));
while *self.token != token::RBRACE {
if self.try_parse_obsolete_with() {
break;
}

self.expect(token::COMMA);
fields.push(self.parse_field(token::COLON));

if self.eat(token::DOTDOT) {
base = Some(self.parse_expr());
if self.try_parse_obsolete_with() {
break;
}

if *self.token == token::RBRACE {
// Accept an optional trailing comma.
if self.eat(token::COMMA) {
if self.eat(token::DOTDOT) {
base = Some(self.parse_expr());
break;
}
} else {
break;
}
fields.push(self.parse_field(token::COLON));
}

hi = pth.span.hi;
Expand Down Expand Up @@ -1709,7 +1714,7 @@ pub impl Parser {

// if we want to allow fn expression argument types to be inferred in
// the future, just have to change parse_arg to parse_fn_block_arg.
let decl = self.parse_fn_decl(|p| p.parse_arg());
let decl = self.parse_fn_decl(|p| p.parse_arg_or_capture_item());

This comment has been minimized.

Copy link
@nikomatsakis

nikomatsakis Feb 27, 2013

likewise (merge failure?)


let body = self.parse_block();

Expand Down Expand Up @@ -1885,10 +1890,13 @@ pub impl Parser {
// For distingishing between record literals and blocks
fn looking_at_record_literal() -> bool {
let lookahead = self.look_ahead(1);
let next_lookahead = self.look_ahead(2);
*self.token == token::LBRACE &&
(self.token_is_keyword(~"mut", lookahead) ||
(is_plain_ident(lookahead) &&
self.look_ahead(2) == token::COLON))
((is_plain_ident(lookahead) &&
next_lookahead == token::COLON) ||
(lookahead == token::RBRACE &&
*self.restriction != RESTRICT_NO_BAR_OR_DOUBLEBAR_OP &&
!can_begin_expr(next_lookahead)))
}

fn parse_record_literal() -> expr_ {
Expand Down
File renamed without changes.
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@
// option. This file may not be copied, modified, or distributed
// except according to those terms.

#[warn(structural_records)];
pub fn main() {
let _foo = {x:5};
}
2 changes: 1 addition & 1 deletion src/test/run-pass/issue-3037.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ enum what { }

fn what_to_str(x: what) -> ~str
{
match x {
match (x) {
}
}

Expand Down
20 changes: 20 additions & 0 deletions src/test/run-pass/struct-empty.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
// Copyright 2013 The Rust Project Developers. See the COPYRIGHT
// file at the top-level directory of this distribution and at
// http://rust-lang.org/COPYRIGHT.
//
// Licensed under the Apache License, Version 2.0 <LICENSE-APACHE or
// http://www.apache.org/licenses/LICENSE-2.0> or the MIT license
// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
// option. This file may not be copied, modified, or distributed
// except according to those terms.



// -*- rust -*-
struct Foo {
}


pub fn main() {
let _foo = Foo{};
}
2 changes: 1 addition & 1 deletion src/test/run-pass/uniq-cc-generic.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ fn make_uniq_closure<A:Owned + Copy>(a: A) -> fn~() -> uint {

fn empty_pointy() -> @mut Pointy {
return @mut Pointy {
mut a : none,
a : none,
d : make_uniq_closure(~"hi")
}
}
Expand Down

2 comments on commit 759702f

@nikomatsakis
Copy link

Choose a reason for hiding this comment

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

This is nice work and much appreciated.

However, I am not sure if your approach to resolving the parser ambiguity is the right one. My personal preference is to say that struct literals cannot appear as match expressions without parentheses (I would say the same thing about following a do; I'm not 100% sure what our rules are there). This is not quite the solution you opted for. I guess this should be discussed at a core team meeting, since this is very much a user-visible thing.

My personal opinion is that it surprises me that match x {} would not parse and I'd rather that match x {} parses than match Foo {} {} or match Foo { x: y } { Foo { x: y } => { ... } }, which are hard to read and which strike me as rather unlikely to occur in practice. If we say that no struct literals can appear as the discriminant of a match, we can also easily report a clear error ("use parentheses" for that case). I'm not sure that empty struct literals even make any sense, perhaps struct Foo {} should be written struct Foo; and handled like an empty enum variant (that is, a nullary constant).

How we choose to resolve this ambiguity also clearly affects how much lookahead we require (though perhaps not in the theoretical sense, if we were to refactor our grammar enough).

@nikomatsakis
Copy link

Choose a reason for hiding this comment

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

(Incidentally, I might be inclined to just merge what you've done and file a follow-up issue to decide how best to resolve the parser ambiguities in this case and others...)

Please sign in to comment.